Repository navigation
fix(duckdb): a remote discover could not read the host's CA bundle on 2.0.0 - #759
Merged
Merged
Conversation
… 2.0.0
`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)
…test stops patching os 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.
…st trust store
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 '<store>' (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.
The adversarial review of dd1aeaa 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 42320db 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 42320db (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.
…annot be trusted 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.
…ree claims are corrected 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.
github-actions
Bot
added
provider
📄 Documentation ReminderThis PR appears to be missing a documentation reference. Our docs live in a separate repo. Please update the PR description with one of:
See the Contributing Guide for details. |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fluid init --discover s3://…and--discover https://…failed on duckdb 2.0.0 before a byte was read: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 filesystem (
CURLOPT_CAINFO_BLOB), where curl and OpenSSL used to open it themselves and bypassedallowed_pathsentirely.Trello: https://trello.com/c/C2fT4zj1 (P0, founder-confirmed 2026-10-27).
What it does
_confine_connectiongrants a private copy of the first CA bundle that exists out of DuckDB's five candidates — one file inallowed_paths, mode 0400 in a 0700mkdtempdirectory,ca_cert_filepinned to it withSET GLOBAL, removed when the connection object is finalised — and only when:allowed_directoriesis untouched, so what a remote prefix grants locally is unchanged (#757). The allowlist object is not widened — the grant is applied atSETtime — whilesandbox_refusal_hintdoes disclose that a copy of the trust store is readable, because saying otherwise would be false.Why a copy, and why those three gates
Each of these was a defect in an earlier version of this PR, found by review and reproduced before fixing:
allowed_pathsis granted for WRITING. The first version granted the host store directly;COPY (SELECT 'ATTACKER CA') TO '<store>'replaced a 333 KB trust store with 14 bytes on 1.5.6 and 2.0.0. Only file ownership stood in the way —/etc/ssl/cert.pemis root-owned — butfluid generate ciemits apython:3.12-slimjob, which runs as root, and that job runsfluid apply;providers/local/local.pyturns a contract'ss3://spec into a remote prefix and then runs that contract's ownproperties.sqlon the connection. A contract could have installed its own CA into the trust store of the container it was built in.mkdtemphonours$TMPDIR, and a workspace-local$TMPDIRis an ordinary CI/direnv/Nix arrangement. There the copy's directory is writable by the SQL, andCOPY TOwith its default options writes a temp file and renames over the target — 333483 bytes → 14, measured on both engines. Hence_ca_dir_within_a_grant, and the grant is dropped with a logged warning. What holds is the reach: the host store is never granted, so this grant writes nothing outside the process's own temp directory.ca_cert_filemust be pinned GLOBAL. A plainSETlands in LOCAL scope, socon.cursor()read''whileallowed_paths(GLOBAL) still showed it the copy: every cursor-based remote read failed on 2.0.0 with the original refusal, and the locked configuration meant no caller could fix it.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 whiles3:///https://go through curl. 1.5.6 exposesenable_curl_server_cert_verification(default true) and 2.0.0 no longer does; duckdb-httpfs#378 proposes unifying them and is still open, so the above is measured rather than quoted. Nothing here disables verification.Reuse
Checked locally first: nothing in
fluid_build/handled a CA bundle (nocert.pem,certifi,SSL_CERT_FILE,ca_cert_file,get_default_verify_paths), and the sandbox already had the single chokepoint this belongs at.http://exemption, and the verification-flag split._openssl.py— the probe-a-candidate-list pattern (get_default_verify_paths()first, then a hardcoded distro list).allowed_paths.Strategy: adapt the pattern, diverge on the list. The candidates mirror DuckDB's own five, in DuckDB's order, not truststore's and not
certifi.where(): what gets copied has to be a store DuckDB itself would have trusted, and DuckDB consults neither OpenSSL's defaults norSSL_CERT_FILE(measured — pointing all three at a granted copy still produced the five-candidate refusal). certifi is installed (viarequests/httpx), but its bundle lives in site-packages, where overwriting it would reach every other tool in the environment.Validation
Live, through the CLI, on 1.5.1, 1.5.6 and 2.0.0:
--discover s3://noaa-ghcn-pds/csv/by_year/2024.csv→ 8 columns,--discover …/duckdb/data/csv/customer.csv→ 18, andfluid validateaccepts the emitted contract.gs://,http://,https://ands3://each read with exactly one granted path. A clean suite run leaves no directory in$TMPDIR.20 tests, all offline except the ones whose subject is the grant (they load httpfs and skip if it cannot be installed). Eleven mutations were reverted and the failures recorded in the test docstring, not predicted — including one count that dropped, and two tests that passed for the wrong reason until they were fixed. The file is registered in the
duckdb-floorlane, whichtest_ci_duckdb_floorrequires of anything branching on the engine version; that gate caught the omission.Full suite at
fdfbe575under xdist, as CI runs it: 28048 passed, 4 failed —test_broad_roots_are_named,test_a_tmpdir_inside_a_project_does_not_make_the_project_a_temp_root,test_dbt_does_not_reload_the_project_dotenv,test_claude_code_live_keyless_envelope_roundtrip, all four pre-existing and all four reproduced on clean5f2ec50b.Review verdicts
Three rounds, every confirmed finding fixed, each reproduced before acting:
pytest -n auto, one red-instead-of-skip offlineKnown limitations (also in the CHANGELOG)
azure:///az://is unmeasured: the azure extension does its own TLS, and on that path the pin cannot land, so no copy is made.is_remote_locationreturns False for any remote URL containing?or#(_REMOTE_RE's path group is/[^?#]*), so presigned URLs and?globs never load httpfs — and the redactor masks everything after?, so the error reads202?***REDACTED***. Alsoabfs://is remote to the duckdb runner but not to the sandbox; confinement refuses it by realpath, so it is a functional gap, not an escape.