[feat](extensions) Add trusted dynamic-extension descriptors - #618
kaka11chen wants to merge 6 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d617fe79d2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ted-extension-descriptors # Conflicts: # DEVELOPMENT.md # cmake/duckdb_loader.cmake # tests/fast/test_loadable_extension_artifacts.py # vane/__init__.py
…ted-extension-descriptors
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 445c9950c6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "LOADED_NAME_CONFLICT", | ||
| f"{candidate.descriptor.name} is already loaded as {existing_name.identity}", | ||
| ) | ||
| if candidate.descriptor.name in database_loaded_names: |
There was a problem hiding this comment.
When a descriptor uses a DuckDB alias such as http, sqlite, or postgres, this raw-name comparison misses an already-loaded canonical name such as httpfs, sqlite_scanner, or postgres_scanner. DuckDB canonicalizes the filename through ApplyExtensionAlias in external/duckdb/src/main/extension/extension_load.cpp before BeginLoad, so the subsequent load becomes a successful no-op and line 487 caches the descriptor's digest even though its bytes were never loaded. Fresh evidence beyond the resolved thread is this mismatch between the resolver's raw-name check and DuckDB's alias normalization; reject aliases or canonicalize names consistently throughout validation and caching.
Useful? React with 👍 / 👎.
| with self._lock: | ||
| resolved = self.resolve(connection, descriptor, artifact=artifact) | ||
| loaded = self._loaded_for_connection(connection) | ||
| database_loaded_names = _database_loaded_extension_names(connection) |
There was a problem hiding this comment.
When two resolver instances target the same database concurrently, both can observe the name as unloaded here because _lock is scoped to one resolver, then race into load_extension; DuckDB's database-wide BeginLoad lets one load win and returns success without loading the other path, after which the losing resolver caches its own digest. Fresh evidence beyond the resolved thread is that the added query and native load remain separate operations at lines 475 and 481, so querying before loading does not close the concurrent-load window.
Useful? React with 👍 / 👎.
| _fail("NAME_MISMATCH", f"{descriptor.identity} must use artifact filename {expected_filename}") | ||
| if not artifact_path.is_file(): | ||
| _fail("ARTIFACT_NOT_FOUND", f"artifact does not exist: {artifact_path}") | ||
| snapshot_path, actual_digest, footer_bytes = self._snapshot_artifact(artifact_path) |
There was a problem hiding this comment.
Every call to resolve(), including a repeated load() whose digest is already cached and every resolution that later fails digest or footer validation, creates another full artifact copy here. These files remain under the resolver's TemporaryDirectory until the resolver itself is destroyed, so a long-lived resolver repeatedly loading a large extension grows temporary-disk usage without bound; reuse cached verified snapshots or delete snapshots that will not be loaded or returned.
Useful? React with 👍 / 👎.
| self._loaded_by_connection: weakref.WeakKeyDictionary[ | ||
| _ExtensionConnection, dict[str, ResolvedDynamicExtension] | ||
| ] = weakref.WeakKeyDictionary() |
There was a problem hiding this comment.
When two distinct protocol-compatible connection objects implement value-based equality and compare equal, WeakKeyDictionary treats them as the same key, so the second connection receives the first connection's cached digests and line 478 can skip loading an extension into a different database. Fresh evidence beyond the earlier weak-cache thread is that Python's WeakKeyDictionary delegates hashing and equality to its referents rather than enforcing the identity hashing claimed by CONNECTION_UNSUPPORTED; use an identity-based weak-key table or reject connection classes whose hash/equality are not identity-based.
Useful? React with 👍 / 👎.
| self._snapshot_directory = tempfile.TemporaryDirectory( | ||
| prefix="vane-dynamic-extensions-", ignore_cleanup_errors=True | ||
| ) |
There was a problem hiding this comment.
When the system temporary directory is mounted with noexec, as in hardened Linux deployments, every verified artifact is copied onto that filesystem and DuckDB's native loader cannot create executable mappings for the shared library, so otherwise valid descriptors fail with LOAD_FAILED. Keep the private snapshot behavior but allow an executable snapshot location to be configured or place snapshots on a filesystem known to permit dynamic-library loading.
Useful? React with 👍 / 👎.
## Summary - Add a strict version-one descriptor, local provider, ordered dependency graph, and fail-closed validation for exact dynamic-extension artifacts. - Delegate extension-name canonicalization, extension-directory and platform discovery, footer parsing, compatibility diagnostics, loading, and loaded-extension provenance to DuckDB native APIs without mutating caller query state. - Preserve DuckDB's full runtime SourceID in descriptors while validating CPP and unstable C-struct footers against DuckDB's native extension compatibility version. - Copy each candidate once into a private content-addressed read-only staging directory, atomically publish the verified directory, reject replaceable cache ancestor chains on POSIX and Windows (including volume-root ownership), coordinate private POSIX creation across concurrent processes under restrictive umasks, and protect every accepted Windows cache directory and snapshot with a current-user-only DACL. - Reject untrusted cache-entry owners, reparse points, and snapshot hard links, and refuse filesystem roots as cache directories. - Use DuckDB's database-wide extension manager as the authoritative load winner, with deterministic conflicts for artifacts loaded from another path or a racing resolver. - Cover validation failures, provider and dependency behavior, cross-platform cache permissions and corruption, concurrent publication and load races, pending connection results, runtime/footer identity separation, and a real staged TPCH extension. The cache isolates operating-system principals. Matching the DuckDB native extension loader and process security model, code running as the same OS identity is inside the process trust boundary. ## Related issue close: #613 Supersedes #618 with the DuckDB-native load-state design. ## Documentation impact - [ ] No user-facing documentation update is required. - [ ] Documentation is updated in this PR or in a linked website PR. - [x] <!-- docs-follow-up --> A follow-up issue is required in `AstroVela/vane-website`. Document descriptor/provider setup, private verified snapshots, database-wide winner semantics, deterministic rejection diagnostics, the same-OS-identity trust boundary, and the explicit unsigned-extension connection setting. ## Validation - `scripts/format root --changed` - `scripts/format duckdb --changed --check` - Non-editable incremental Release install in `build/python-release` - `cmake --build build/python-release --target vane_loadable_extension_tpch` - `VANE_TEST_LOADABLE_EXTENSION_PATH="$PWD/build/python-release/vane_extensions/tpch.duckdb_extension" scripts/run_installed_pytest.sh tests/fast/test_dynamic_extension_resolver.py` — 39 passed - Release non-Ray shard — 694 passed / 18 deselected; 3 existing Ray bounded-diagnostic assertions fail because their short Python errors are replaced by the 4096-byte omission message in this shared environment - A/B baseline check — the same 3 failures reproduce with the cached `323f35c793` installation that contains the pre-feedback loader implementation; neither the failing tests nor their implementation paths overlap this PR - Release real-Ray shard — 14 passed / 701 deselected ## Checklist - [x] The change is focused and includes tests or a reason tests are unnecessary. - [x] Public behavior and compatibility impact are documented. - [x] No new dependencies, copied code, model assets, or datasets are introduced. - [x] No credentials, private endpoints, personal paths, generated data, model weights, or build artifacts are included. - [x] Security implications of UDFs, serialization, remote code, network access, and untrusted input have been considered. - [x] Native changes were compiled; Python-only, documentation, and workflow changes passed relevant checks.
Summary
Related issue
close: #613
Documentation impact
AstroVela/vane-website.Document descriptor/provider setup, verified snapshot loading, deterministic rejection diagnostics, and the explicit unsigned-extension connection setting.
Validation
scripts/format root --changed --checkpre-commit run --files src/vane_py/vane_python.cpp vane/_native/__init__.pyi vane/__init__.py vane/extensions.py tests/fast/test_dynamic_extension_resolver.pybuild/python-releasecmake --build build/python-release --target vane_loadable_extensions --parallel 12tests/fast/test_dynamic_extension_resolver.py: 16 passed, including the staged TPCH artifactThe base suite was also used to diagnose a local long-worktree-path interaction with the existing 4096-byte Ray diagnostic bound; the same installed artifacts pass the complete suite through a short
/tmppath alias.Checklist