fix(clickhouse): honor verify=false in the Rust HTTP writer by bhataprameya · Pull Request #8419 · getsentry/snuba

bhataprameya · 2026-08-29T08:29:41Z

Context

Rust consumers always verify ClickHouse TLS certificates, while the Python clients honor the cluster verify setting (CLICKHOUSE_VERIFY). With secure HTTPS and a self-signed/private-CA certificate, Rust consumers fail with certificate verify failed: unable to get local issuer certificate even when verification is explicitly disabled.
Scope: this only honors an explicit verify=false. Unset CLICKHOUSE_VERIFY is unchanged: Python skips verification, Rust still verifies, so CLICKHOUSE_SECURE=true with the env var unset still fails in the Rust consumer.

Changes

Coerce the raw CLICKHOUSE_VERIFY env string once in ClickhouseCluster.get_verify() (only explicit false/0 disable; None stays None) so Python clients and the Rust consumer config see the same value.
Serialize verify into the Rust consumer config; the writer disables certificate and hostname verification (tls_danger_accept_invalid_certs + tls_danger_accept_invalid_hostnames, matching Python's CERT_NONE) only when secure && verify == Some(false), and logs a warning when it does.
Cluster builders keep an omitted verify key as None instead of False so "unset" stays distinct from "explicitly disabled" (both are falsy at existing Python call sites, so no behavior change).
ca_certs is still not passed to the Rust writer, so a private CA cannot be trusted properly yet. That is a follow-up, not part of this PR.

Testing

cargo test --lib: serde defaults (None when absent, Some(false) when explicit) and the tls_verification_disabled gate matrix.
pytest tests/clusters/test_verify.py tests/consumers/test_consumer_config.py: get_verify coercion matrix; explicit False propagation through resolve_storage_config/asdict.
cargo clippy --all-targets -- -D warnings, cargo fmt --check, ruff, mypy: clean.

Legal Boilerplate

Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.

The Rust consumer writer always verified ClickHouse TLS certificates, while the Python clients honor the cluster verify setting. Plumb verify into the Rust consumer config and disable certificate and hostname checks only for HTTPS when verify is explicitly false.

phacops

Thanks for cutting this down — leaving the Python default alone is the important difference vs last time. Fail-closed on the Rust side (None keeps verifying) is what we want.

A few things before this can land:

The description overstates what this actually does. Unset CLICKHOUSE_VERIFY is still None, Python still skips verification, and Rust still verifies. The only path this fixes is an explicit false. That's fine, but please say so in the PR body — CLICKHOUSE_SECURE=true with the env var unset will still fail in the Rust consumer.

_coerce_verify also only runs when we serialize for Rust. CLICKHOUSE_VERIFY comes in as a string from the environment, and the Python clients still treat "false" as truthy. If we're going to coerce, it should live on ClickhouseCluster.get_verify() so everybody sees the same value. Just don't turn None into True.

Tests are thin for something that turns off TLS checks. serde of the config field + a parametrize of the private helper doesn't cover the writer, and assert verify is None on the default fixture isn't really propagation. Need a test that False actually flows through resolve_storage_config, and something that covers the secure && verify == Some(false) gate (pulling that condition out into a tiny helper is fine).

A tracing::warn when we disable verification would also help — easy to miss otherwise.

(We're still not passing ca_certs through, so a private CA still can't be trusted properly. That's a follow-up, not a blocker.)

phacops · 2026-09-01T13:52:09Z

+            builder = builder
+                .tls_danger_accept_invalid_certs(true)
+                .tls_danger_accept_invalid_hostnames(true);
+        }


This is the bit that actually matters and it isn't tested — the config.rs tests only check serde.

tls_danger_accept_invalid_hostnames is also a bigger hammer than "accept my private CA". It matches Python's CERT_NONE, so I'm not asking you to drop it, but the comment should say we disable hostname checks too, not just an untrusted issuer.

A warn log here would be good.

Done. The condition is now a tls_verification_disabled helper with a matrix test covering all four secure/verify combinations, the comment calls out that hostname checks are disabled too, and there's a tracing::warn when verification is off.

phacops · 2026-09-01T13:52:09Z

+    # only an explicit false disables verification.
+    if isinstance(verify, str):
+        return verify.strip().lower() not in ("false", "0")
+    return verify


This only affects the Rust payload. Python HTTPBatchWriter is still cert_reqs="REQUIRED" if verify else "CERT_NONE" and clickhouse-connect does bool(self.verify), so a string "false" from the env keeps verifying on the Python side.

Can we coerce once in get_verify() instead? Keep None as None.

Moved the coercion into ClickhouseCluster.get_verify(): only explicit false/0 disable, None stays None. The pool call sites now use get_verify() too, so Python clients and the Rust payload see the same value.

phacops · 2026-09-01T13:52:09Z


    assert len(resolved.storages) == 1
    assert resolved.storages[0].clickhouse_table_name in ("errors_local", "errors_dist")
+    assert resolved.storages[0].clickhouse_cluster.verify is None


This is just the default. Doesn't show that an explicit False survives resolve_storage_config / asdict.

Added test_resolve_storage_config_propagates_verify_false, which asserts an explicit False survives resolve_storage_config and lands as false in the asdict payload.

Coerce the raw CLICKHOUSE_VERIFY env string in ClickhouseCluster.get_verify so Python clients and the Rust consumer config see the same value; None stays None. Extract the secure/verify gate into tls_verification_disabled with a matrix test, log a warning when verification is disabled, and test that an explicit False survives resolve_storage_config.

bhataprameya · 2026-09-02T00:18:00Z

Thanks for the review. All addressed in ecefd9f:

PR body now states the actual scope: only an explicit verify=false is honored; unset CLICKHOUSE_VERIFY is unchanged (Python skips, Rust verifies, so CLICKHOUSE_SECURE=true with the env var unset still fails in the Rust consumer).
Coercion moved into ClickhouseCluster.get_verify(): only explicit false/0 disable, None stays None. The pool call sites (get_node_connection, get_batch_writer) now use get_verify() too, so Python clients and the Rust payload see the same value.
The secure && verify == Some(false) condition is now a tls_verification_disabled helper with a matrix test, plus a tracing::warn when verification is disabled. The comment notes hostname checks are disabled as well.
test_resolve_storage_config_propagates_verify_false covers an explicit False flowing through resolve_storage_config into the asdict payload; tests/clusters/test_verify.py covers the get_verify coercion matrix.
Noted in the PR body that ca_certs is still not plumbed to Rust; agreed, follow-up.

pbhandari · 2026-09-03T15:00:53Z

        secure: bool,
        ca_certs: str | None,
-        verify: bool | None,
+        verify: bool | str | None,


why do we need this to also be a str?

pbhandari · 2026-09-03T15:03:07Z

+        if isinstance(verify, str):
+            return verify.strip().lower() not in ("false", "0")
+        return verify


I really would MUCH prefer if __verify just couldn't be passed in as a string, but if it NEEDS to be a string, then we should coerce it into a bool at the constructor level, or earlier, and have this function go back to doing what it used to do before.

pbhandari · 2026-09-03T15:04:20Z

        secure=cluster.get("secure", False),
        ca_certs=cluster.get("ca_certs", None),
-        verify=cluster.get("verify", False),
+        verify=cluster.get("verify"),


Suggested change

verify=cluster.get("verify"),

verify=cluster.get("verify", True),

Much better to be explicit about the expectations, this way anyone can tell at a quick glance that the default is True.

pbhandari · 2026-09-03T15:04:47Z

        secure=cluster.get("secure", False),
        ca_certs=cluster.get("ca_certs", None),
-        verify=cluster.get("verify", False),
+        verify=cluster.get("verify"),


Suggested change

verify=cluster.get("verify"),

verify=cluster.get("verify", True),

same reasoning as this comment: https://github.com/getsentry/snuba/pull/8419/changes#r3925857030

pbhandari · 2026-09-03T15:06:49Z

    password: str
    database: str
    secure: bool
+    verify: bool | None


do we need this to be nullable? I don't personally see much value in that, and given we want None to mean verify = True, I can see this being a bit of a footgun in Python code.

pbhandari · 2026-09-03T15:08:22Z

+    assert resolved.clickhouse_cluster.verify is False
+    assert dataclasses.asdict(resolved)["clickhouse_cluster"]["verify"] is False


why do we need both these asserts?

pbhandari · 2026-09-03T15:10:23Z

+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    storage = get_writable_storage(StorageKey.ERRORS)
+    monkeypatch.setattr(storage.get_cluster(), "get_verify", lambda: False)


This feels needlessly brittle. Is it possible to test this without monkeypatching?

bhataprameya requested a review from a team as a code owner August 29, 2026 08:29

phacops requested changes Sep 1, 2026

View reviewed changes

bhataprameya requested a review from phacops September 2, 2026 00:16

pbhandari requested changes Sep 3, 2026

View reviewed changes

	verify=cluster.get("verify"),
	verify=cluster.get("verify", True),

		assert resolved.clickhouse_cluster.verify is False
		assert dataclasses.asdict(resolved)["clickhouse_cluster"]["verify"] is False

Uh oh!

Uh oh!

Conversation

bhataprameya commented Aug 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Changes

Testing

Legal Boilerplate

Uh oh!

phacops left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bhataprameya Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bhataprameya Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bhataprameya commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bhataprameya commented Aug 29, 2026 •

edited

Loading

bhataprameya Sep 2, 2026 •

edited

Loading

bhataprameya Sep 2, 2026 •

edited

Loading

bhataprameya commented Sep 2, 2026 •

edited

Loading