feat: tower-duckdb crate — sandboxed catalog queries and adversarial tests#331
Open
bradhe wants to merge 4 commits into
Open
feat: tower-duckdb crate — sandboxed catalog queries and adversarial tests#331bradhe wants to merge 4 commits into
bradhe wants to merge 4 commits into
Conversation
…ests Agent-issued catalog SQL is untrusted, so before an MCP query tool can run it we need the query path locked down and proof that the lockdown holds. This lands that foundation without wiring it to a caller yet. It adds a `sandbox` module with the gates a future `tower_catalogs_query` tool applies: reject write/DDL and multi-statement input (the multi-statement scanner skips separators inside strings and comments, which matters because duckdb-rs `prepare` executes every statement but the last as a side effect), and a session hardening set that disables local-filesystem access, blocks community extensions, and locks the configuration. The hardening disables only `LocalFileSystem`, so httpfs and the object-store reads an attached Iceberg catalog depends on keep working. `run_duckdb_query` gains an optional row cap that flags the result truncated, so a model cannot pull an unbounded table into memory or its context. The adversarial suite is the point: each test encodes an attack an agent query might attempt (data tampering, statement smuggling, host filesystem reads and writes, configuration escape, arbitrary extension loading, SSRF via table functions) and asserts the sandbox refuses it. A testcontainers MinIO test proves the same hardening does not break a real object-store Iceberg read while local access and config changes stay blocked, and self-skips when no Docker daemon is present so a plain `cargo test` still passes.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…crate and wire the sandbox in The sandbox primitives from the previous commit had no caller: the gates, the session hardening, and the row cap all lived in an inline `#[allow(dead_code)]` module in catalogs.rs, proven only by tests. This moves Tower's DuckDB usage into a dedicated `tower-duckdb` crate and makes `tower catalogs query` its first real consumer, so the hardening runs in production rather than sitting on a shelf. The crate owns the whole DuckDB surface: opening an in-memory Session, running trusted setup, locking the session down for untrusted SQL (Session::harden), and executing a query into JSON rows with an optional row cap. The static text gates (reject write/DDL, reject multi-statement) live in its `guard` module. The value conversion, the query execution, and the full adversarial + integration test suite move with it, so the security invariants are tested next to the code they protect. tower-cmd no longer depends on duckdb directly; it goes through the crate. `catalogs query` now splits by mode. Read mode is the default and the sandboxed path: the SQL is gated before it reaches DuckDB (a smuggled second statement would otherwise run as a side effect of prepare, and a write gets a clear message instead of a raw engine error), the session is hardened after attach, and rows are capped with a truncation notice. Write mode stays the trusted power-user opt-in and runs as before. Once this lands, #328 will layer the MCP `tower_catalogs_query` tool on top of the same crate.
bradhe
changed the title
konstantinoscs
requested changes
Jul 24, 2026
konstantinoscs
left a comment
konstantinoscs
left a comment
Contributor
There was a problem hiding this comment.
The SQL injection please. The rest is ok!
… keyword denylist The read-only gate checked only the first SQL keyword against a denylist. That is not a safe policy: a `--` comment ends at a carriage return as well as a newline in DuckDB, so a `-- x\rDROP …` payload looked empty to the scanner but parses as a DROP, and a statement that opens with an allowed keyword (a `WITH` CTE, for one) can still mutate. Addresses Konstantinos's review on #331. The gate now runs the SQL through DuckDB's own parser via `json_serialize_sql`, which parses without executing and serializes only SELECT statements, erroring on anything else. `classify_read_only` returns Allowed only for exactly one SELECT; everything else (writes, DDL, PRAGMA/SET, multi-statement, unparseable) is refused, fail-closed. This is an allowlist of what the executor will actually run, so the comment-terminator and CTE bypasses are caught, and the SQL is bound as a parameter rather than spliced into the parser query. The keyword denylist and the hand-rolled statement scanner are gone, along with their unit tests; new tests cover the single-SELECT allowlist, the `\r` smuggling case, the mutating-CTE case, and empty/multiple classification.
…ere it can't install
The Windows test job failed because the read-only gate runs `json_serialize_sql`,
which needs the `json` extension. It is not bundled, so DuckDB tried to
auto-download and install it, and the install fails on the Windows runner ("Could
not move file: Access is denied"). That is a real defect, not just a test
problem: the gate would be broken for Windows users too.
Enabling the duckdb `json` feature compiles the extension statically into the
bundled build, so it is available with no autoload or network. Verified it works
with `autoinstall_known_extensions` and `autoload_known_extensions` both off.
The `iceberg_scan` regression test needs the `iceberg` extension, which has no
such feature and must still be fetched at runtime. It now self-skips when that
install cannot happen, the same way the object-store test skips without Docker,
so the Windows job stops failing on an environment it can't satisfy while the
test keeps running where it can.
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
2 participants
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.
What this is
This PR adds a
tower-duckdbcrate that owns Tower's usage of DuckDB, and wires its query sandbox intotower catalogs query. It carries Konstantinos's review feedback from #328 (read-only enforcement, session sandboxing, bounded results) forward as a reusable, tested foundation. The MCPtower_catalogs_querytool that also consumes the crate lands in a follow-up on #328.Why it matters
Once an agent can issue SQL against a customer's catalog, the query text is untrusted input running on the customer's machine with their credentials. Read-only credentials are not enough on their own: a query can still read the local filesystem through
read_csv('/etc/passwd'), reach internal network endpoints through table functions, load an arbitrary extension, or smuggle a second statement past a naive single-statement check. This PR closes that surface, puts the whole DuckDB integration in one crate so there is a single place to reason about it, and pins the invariants down with tests so a future change cannot quietly reopen them.The crate
tower-duckdbis the whole DuckDB surface in one place:Session— open an in-memory connection,run_setup(trusted statements: attach, load, secrets),harden(lock the session down for untrusted SQL), andquery(execute one statement into JSON rows with an optional row cap that flags truncation).guard— the static text gates:is_write_statement,is_multi_statement(skips;inside strings and comments, which matters because duckdb-rsprepareruns every statement but the last as a side effect), andAGENT_MAX_ROWS.hardening_statements— disable local-filesystem access, block community extensions, lock configuration. OnlyLocalFileSystemis disabled, so httpfs and the object-store reads an attached Iceberg catalog depends on keep working.value_to_json— DuckDB value toserde_json, rendering wide integers and temporal types as strings so no precision is lost.tower-cmd no longer depends on
duckdbdirectly; it goes through the crate.The wiring
tower catalogs querynow splits by mode:prepare; a write statement gets a clear message instead of a raw engine error), the session is hardened after attach, and rows are capped with a truncation notice.--write) stays the trusted power-user opt-in and runs as before.Tests
The adversarial suite lives with the code it protects, in
tower-duckdb. Each test encodes an attack an agent query might attempt and asserts the sandbox refuses it: data tampering, statement smuggling, host filesystem reads and writes, configuration escape, arbitrary extension loading, and SSRF via table functions. A positive-control test confirms a plainSELECTstill runs under the hardened session. A testcontainers MinIO integration test proves the same hardening does not break a real object-store Iceberg read while local access and config changes stay blocked; it self-skips when no Docker daemon is present, so a plaincargo teststill passes. A local Iceberg regression test writes a real Iceberg table withCOPY (FORMAT iceberg)and reads it back.Relationship to #328
This supersedes #328 ("Add query support to local MCP server"). The MCP query tool will be re-added on top of this crate, reusing the same
Session/guardpath so the agent tool and the CLI share one hardened implementation.