Let the ClickHouse connector work with a token that cannot read the system database - #1344
Open
adamparrott-ls wants to merge 3 commits into
Open
Let the ClickHouse connector work with a token that cannot read the system database#1344adamparrott-ls wants to merge 3 commits into
adamparrott-ls wants to merge 3 commits into
Conversation
Tinybird serves the system database to ADMIN tokens only, so a token scoped to a few tables cannot be used with nao at all. Behind a new opt-in tolerate_unreadable_system_tables field: - connection survives an unreadable system.settings, which clickhouse-connect reads while constructing the client - schema and table discovery fall back to the tables named outright in include, via a wrapper around the Ibis backend - column metadata falls back to DESCRIBE TABLE, which needs no system access Nothing changes with the field off, and the fallbacks are only reached when the underlying call fails, so a server that serves the system database keeps full listings and system.columns metadata. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
- warn about unexpandable include patterns from list_databases too, so an include made only of wildcards reports the patterns it is dropping instead of syncing zero schemas silently - escape backticks when quoting identifiers for DESCRIBE TABLE - gate the DESCRIBE fallback on tolerate_unreadable_system_tables, so an empty system.columns result stays empty on the default path - hold the server settings probe lock when connecting without the opt-in, so a concurrent opt-in connection's patch cannot leak into it Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Author
|
Cubic review validated, changes pushed. |
Author
|
@Bl3f we'd still be keen to get this merged in to reduce the blast radius on access to our warehouse. Please let me know if there's anything I can do to help get this moved forwards. |
Contributor
|
@adamparrott-ls hello Adam, sorry for being a bit slow. I'll try to merge this asap, the thing i need to understand is what's the impact for people using already Clickhouse? I'm not sure while reading the PR that add a significant amount of code for it. |
Author
|
There is no impact for existing clickhouse users. There is a new config
field that enables a user that wants to use a scoped token to I accept the
lack of system table availability. Even if
the tolerate_unreadable_system_tables is true, reading system tables is
still attempted and falls back to a describe on the table is named in the
include.
…On Fri, 28 Aug 2026 at 06:45, Christophe Blefari ***@***.***> wrote:
*Bl3f* left a comment (getnao/nao#1344)
<#1344 (comment)>
@adamparrott-ls <https://github.com/adamparrott-ls> hello Adam, sorry for
being a bit slow. I'll try to merge this asap, the thing i need to
understand is what's the impact for people using already Clickhouse?
—
Reply to this email directly, view it on GitHub
<#1344?email_source=notifications&email_token=CJGY5PEL4QLV52YVMNGNFKT5MEL6TA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKNBUHA4TENJWHA32M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-5448925687>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/CJGY5PCLA37LFCSFKUTXBQT5MEL6TAVCNFSNUABGKJSXA33TNF2G64TZHMYTCMRVGQ2DKNJVGA5US43TOVSTWNJQGU4DKMRXGM3DJILWAI>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/CJGY5PDLHJFFOOZGSHCLAET5MEL6TA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKNBUHA4TENJWHA32M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJKTGN5XXIZLSL5UW64Y>
and Android
<https://github.com/notifications/mobile/android/CJGY5PBU2DPFWAH6QDLM2RT5MEL6TA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKNBUHA4TENJWHA32M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLTGN5XXIZLSL5QW4ZDSN5UWI>.
Download it today!
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
ty rejects assigning the wrapper over HttpClient.query as an implicit shadowing of the declared method, which failed the CLI Lint job. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Closes #1319.
Tinybird serves the
systemdatabase to ADMIN tokens only, so a token scoped to a few tablescan't be used with nao at all.
system.settingswas just the first wall —system.databasesand
system.tablesare blocked too, so fixing the connect-time read alone only moves thefailure one step.
The practical effect is that the only credential that can connect is a workspace admin token, meaning that if a guardrail was not fully realised, then a create, update, or delete statement would be actioned on a production database.
This adds one opt-in field,
tolerate_unreadable_system_tables(defaultfalse), which doesthree things when the system database is unreadable:
system.settings. clickhouse-connect reads it insideClient.__init__to validate query settings, so there's no client to configure beforehand —the query method is wrapped for the length of the connection, matching that one query and
nothing else. A query you write against
system.settingsstill fails as it should.include, via a thin wrapper aroundthe Ibis backend. No change to the shared sync path, so no other backend is affected.
Wildcards can't be expanded without a listing, so restricted-token users must name each
table; any pattern that gets skipped is logged rather than silently dropped.
DESCRIBE TABLE, which returns the same name, type,comment and default fields as
system.columnsand needs no system access.Nothing changes with the flag off, and nothing is lost with it on: the fallbacks are only
reached when the underlying call actually fails, so a server that serves the system database
still gets full listings and
system.columnsmetadata.What a Tinybird token actually needs
This is the second half of #1319 — the minimum-privilege set, established by testing rather than
by reading docs. With the field enabled, nao needs only:
SELECTon each datasource named inincludeDESCRIBE TABLEon those datasources (column names, types, comments, defaults)No system-database access at all.
SELECT version(), timezone()is permitted to any token.system.projectionsdoesn't exist on Tinybird, and every other system read nao attempts alreadydegrades to
None, costing only the table comment and the engine/index metadata.Why not just scope the token instead
Tinybird's error suggests adding the resource to a token with
DATASOURCES:READ. That doesn'twork for
system.settings— it can't be granted to a non-ADMIN token, so an ADMIN tokenbelonging to a named individual is currently the only credential that can connect. That's the
blast radius the issue is about.
Testing
Against a real Tinybird workspace with a read-only token, and a local ClickHouse for regression:
nao syncfrom the CLI: 15/15 tables across 2 datasets, no errorscolumns,preview,profiling,ai_summary. Profilingproduced real per-column stats (min/max/distinct/nulls/stddev, top values for
LowCardinality) over a 242k-row table/execute_sqlon the FastAPI sidecar (the chat path): real aggregates returned, dialectdetected as
clickhouseclickhouse-connect 1.6.0 and 0.14.1
cliunit suite: same 25 pre-existing env-related failures before and after, +23 newtests passing
clickhouseextra installedruff check,ruff check --select I,ruff format --checkclean apart from pre-existingfindings
Notes for review
already tolerates the adjacent
client_protocol_versionprobe failing(
_backend/orchestration.py); the same treatment for the settings probe would remove the needfor that piece. Happy to send that upstream separately — parts 2 and 3 belong here regardless.
DATABASE_CONFIGSinscripts/generate-config-docs.py(nor MySQL, Fabricor StarRocks), so no ClickHouse option appears in the generated config reference and this new
one won't either. Left alone here since it's pre-existing and unrelated — happy to raise it
separately if useful.
includeentry naming a table thatdoesn't exist is silently filtered out. When discovery falls back to
include, the name istrusted, so it surfaces as a logged per-table error and a
columns.mdcontaining that errorinstead. Louder rather than silent, which seems right when
includeis the source of truth,but it is a difference.
BLE001blind-excepts, consistent with the 13 already in this file.tywantssetattrwhere ruff's B010 wants direct assignment; I went with ruff, since that's what CIruns.
This PR was written using Claude Opus 5 (claude-opus-5).