security(clickhouse): keep client-side readonly off server-enforced reads

[phacops]

Ref VULN-2528.

This PR changed direction. It previously made Snuba send readonly=2 on its read profiles. Now that the API deployment's CLICKHOUSE_USER is provisioned with a server-side readonly=2 profile — as the admin read tools' CLICKHOUSE_READONLY_USER already is — that approach is not just redundant, it would break every read:

Cannot modify 'readonly' setting in readonly mode

So the client-side work is reverted in full. What remains is a guard so it cannot come back, plus devservices support for the constrained user.

Why server-side won

Read-only can be enforced two ways, and they are mutually exclusive on the same connection:

	Enforced by	Can a query escape it?
Client-side (readonly=2 in query settings)	Snuba, per query	Yes — any read path built without the profile misses it
Server-side (the ClickHouse user's profile)	ClickHouse	No

Server-side is strictly stronger, and it turned out to be available: the API process never writes to ClickHouse, so its CLICKHOUSE_USER can be read-only without affecting ingestion. Its DELETE endpoints (views.py:340, endpoint_delete_trace_items.py:160) produce to Kafka; the actual DELETE FROM runs in the separate lw_deletions consumer. Migrations, the replacer, optimize and CLEANUP are all separate deployments on the writable credential.

Note for provisioning: that user needs readonly=2, not readonly=1. Reads send per-query settings (max_threads, load_balancing, query_id) and readonly=1 rejects any setting change.

What is in this PR

• A test pinning that the QUERY and QUERYLOG profiles send no readonly setting, with the reasoning inline, so re-adding one fails in CI rather than in production. This is the durable part: the whole detour happened because nothing encoded which paths are server-enforced.
• devservices/clickhouse/users.xml gains a readonly user on a readonly=2 profile, so the constrained path can be exercised locally and the expected production shape is documented.

What was reverted

• readonly=2 on QUERY and QUERYLOG
• The ADMIN_READ profile and the admin call-site changes, which existed only to shield the admin read paths from QUERY's readonly=2
• The ADMIN_SUDO profile, which existed only to keep sudo statements off a readonly profile

TRACING and CARDINALITY_ANALYZER keep their pre-existing readonly=2. They authenticate as CLICKHOUSE_TRACE_USER, which is not constrained server-side, so for them the client-side setting is the only guard there is. If that user is ever given a readonly profile, they will need the same treatment — worth a follow-up.

Known trade-off

Deployments that do not provision a read-only user for the API — self-hosted, most likely — no longer get the client-side readonly=2 and so keep running reads at readonly=0. That is a deliberate cost of the two mechanisms being mutually exclusive: Snuba cannot know whether the user it authenticates as is already constrained, so it cannot safely send the setting "just in case". The actual injection VULN-2528 reported is fixed at its source in getsentry/sentry#121911, which applies to every deployment regardless.

Testing

The suite passes locally (46 tests in tests/clickhouse/test_connect.py; the one error there is a pre-existing ClickHouse-dependent test, and no ClickHouse or Docker daemon was available in my environment). Nothing here exercises a real constrained user yet — wiring a CI job for that is follow-up, since the test suite writes to ClickHouse via migrations and fixtures and so cannot be pointed at a read-only user wholesale.

Not included

The ClickHouse function allowlist from an earlier revision of this branch was dropped. The injection it backstopped is fixed in getsentry/sentry#121911, and AllowedFunctionValidator defaults to off, so the list had no runtime effect. Turning that validator on needs its own soak against the invalid_funcs metric. For whoever picks it up: of the five values Sentry now permits, only identity is missing from Snuba's list today.

[phacops]

The alias finding was correct, and it's structural rather than a gap to patch — CURRENT_USER and fullHostName lowercase to names the denylist never listed, and no enumeration of ClickHouse aliases stays complete.

Fixed by dropping the denylist and enforcing the GLOBAL_VALID_FUNCTIONS allowlist instead (604ca51). An allowlist rejects every alias for free by never listing it, and also rejects helpers the denylist had to permit, like reinterpretAsUInt64. The allowlist was measured against its callers before being enforced — 127 function names emitted by getsentry/sentry, plus Snuba's own entity function mappers — and function-validator.enabled is now a kill switch rather than an opt-in gate. Details in the updated PR description.

---

Generated by Claude Code

[phacops]

Correcting my comment above before it misleads a reviewer: function-validator.enabled is not a kill switch, and enforcement is not on by default.

That revision was reverted. The option keeps its shipped default: false and stays an opt-in gate, to be turned on out of band, so there is no options-schema change left in this PR. Practically: on merge, an unlisted function name is still only counted, and the injection stays open on the Snuba side until the option is set. The allowlist work here is what makes that flip safe rather than breaking legitimate queries.

The PR description is current; the earlier comment is not.

---

Generated by Claude Code

[linear-code]

VULN-2528

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 4ed244d. Configure here.}

[cursor]

QUERY treated as server-side readonly

High Severity

The new test forbids readonly on QUERY on the claim that API CLICKHOUSE_USER is already server-side readonly. QUERY is still what get_reader() uses, and that path authenticates as the writable cluster user shared with get_batch_writer(), not CLICKHOUSE_READONLY_USER. The local readonly user is only reachable by setting CLICKHOUSE_READONLY_USER, which does not change user-facing reads. That locks in VULN-2528 instead of applying client-side readonly=2 on the user-facing path.

Additional Locations (1)

• devservices/clickhouse/users.xml#L9-L32

 

^{Reviewed by Cursor Bugbot for commit 4ed244d. Configure here.}

[phacops]

Half right, and the half that's right was a real error — fixed in 189771a.

Correct, and my mistake: the devservices addition could not do what its comment claimed. I wrote that setting CLICKHOUSE_READONLY_USER=readonly would exercise "the user-facing read path", which is false — that variable only selects credentials for the admin helpers in snuba/admin/clickhouse/common.py. The user-facing path authenticates as the cluster credential from CLUSTERS, i.e. CLICKHOUSE_USER, exactly as you say.

It also could not have worked even with the right variable: devservices/config.yml mounts that file as the container's users.xml, and the snuba service there runs devserver — API, consumers and ENABLE_AUTORUN_MIGRATION_SEARCH_ISSUES: 1 — all under one credential, so constraining it would break migrations and every consumer locally. CI doesn't mount the file at all. The whole addition was inert at best and misleading at worst, so it's dropped. The PR is now the single test.

Where I disagree: the test does not lock in VULN-2528. You're right that QUERY is what get_reader() uses and that nothing in this repository points it at a constrained user — that's precisely the point. Read-only for that path is being enforced by provisioning: the API deployment's CLICKHOUSE_USER is set to a user with a server-side readonly=2 profile. That's config you can't see from here, which is why the conclusion looks wrong from inside the diff.

To be straight about the evidence: I'm relying on the maintainer's statement that this provisioning is now in place, not on anything I verified myself. If it turns out not to be, this test is wrong and readonly=2 should go back on QUERY — so the test's docstring names the assumption explicitly rather than leaving it implicit.

The two mechanisms cannot both be used. Sending readonly=2 to a user already in readonly mode fails with Cannot modify 'readonly' setting in readonly mode, and a readonly=1 profile rejects any setting at all — which is the same failure mode you correctly flagged on the admin paths earlier in this PR. Server-side is also strictly stronger, since a query cannot opt out of it.

---

Generated by Claude Code

[phacops]

Closing — nothing worth merging is left here.

VULN-2528 is addressed in two other places instead:

• The injection itself is fixed at its source in ref(discover): Restrict the fn argument of fn_span_count sentry#121911, which constrains fn_span_count's fn argument to ["identity", "sum", "avg", "min", "max"].
• The readonly=0 finding is handled by provisioning: the Snuba API deployment's CLICKHOUSE_USER runs with a server-side readonly=2 profile. Server-side enforcement is strictly stronger than a setting Snuba sends, since a query cannot opt out of it — and the two are mutually exclusive, because sending readonly to a user already in readonly mode fails with Cannot modify 'readonly' setting in readonly mode.

That left this PR with a single test asserting behaviour that is already true on master, which is not worth carrying.

For anyone arriving from the ticket, the parts worth remembering:

• Client-side and server-side readonly cannot be combined on the same connection.
• A read-only ClickHouse user for Snuba's read paths needs readonly=2, not readonly=1: reads send per-query settings (max_threads, load_balancing, query_id) and readonly=1 rejects any setting change.
• TRACING and CARDINALITY_ANALYZER still send readonly=2 as a client setting on CLICKHOUSE_TRACE_USER. If that user is ever given a server-side readonly profile, those admin tools break the same way — a live follow-up.
• The ClickHouse function allowlist that appeared on an early revision of this branch was dropped. AllowedFunctionValidator defaults to off, so it had no runtime effect; enabling it needs its own soak against the invalid_funcs metric. Note for whoever does that: of the five values Sentry now permits, only identity is missing from Snuba's list today.

Thanks to Cursor Bugbot, which caught two genuine problems on this branch: readonly=2 breaking the admin read tools, and a devservices addition whose comment described behaviour it did not have.

---

Generated by Claude Code