TCP B9: Add TLS to the native client

[alex-clickhouse]

Stacked on #562 (tcp/epic-j5-json). Review that one first — this diff is only the TLS work.

Closes epic story B9, plus a Cloud smoke suite over the new transport.

What this does

ClickHouseTcpConnection.ConnectAsync takes a TlsParameters (null = plaintext) and wraps the NetworkStream in an SslStream between the socket connect and the protocol handshake. TLS sits below the protocol, so nothing above the transport changes — the native protocol sends the same bytes either way. What it buys is that the ClientHello, which carries the password as plaintext, is already inside the tunnel.

Options are flat on ClickHouseTcpClientOptions, each with a matching connection-string key:

Option	Connection-string key	Meaning
UseTls	UseTls	Encrypt the transport
TlsServerName	TlsServerName	SNI and certificate name; defaults to Host
TlsAllowInvalidCertificates	TlsAllowInvalidCertificates	Development skip-verify
TlsCaCertificatePath	TlsCaCertificatePath	PEM authority file to validate against
ConfigureTls	— (code only)	Action<SslClientAuthenticationOptions> escape hatch, applied last

await using var client = new ClickHouseTcpClient("Host=abc.clickhouse.cloud;UseTls=true;Password=…");

Decisions worth a reviewer's attention

Port is now int?. ResolvedPort derives 9440 with TLS and 9000 without; an explicit value always wins, so TLS on a non-standard port needs no other change. This is a source-breaking change to a public property — acceptable because ClickHouse.Driver.Tcp is IsPackable=false, has no PublicAPI/*.txt baseline yet (Epic R1), and is gated behind the experimental diagnostic.

A pinned authority replaces the host trust store rather than adding to it. A certificate the host would accept on its own is refused unless it also chains to the named authority. An additive check would still take a certificate mis-issued by any public authority, which is the attack pinning exists to stop. The rebuild also keeps the two checks the platform would have made — the serverAuth EKU, and the revocation mode the TLS options carry — because it replaces the platform's check rather than supplementing it.

Three guards against silently connecting in the clear. Validate() refuses any Tls* property set while UseTls is false (a forgotten UseTls with a configured CA is exactly the mistake worth catching), refuses TlsAllowInvalidCertificates together with TlsCaCertificatePath as contradictory, and the connection-string boolean getter throws on a value it cannot read instead of falling back to false — UseTls=yes reading as false would hand back a plaintext connection the caller believes is secure.

mTLS is not implemented. Client certificates are reachable only through ConfigureTls. A declarative surface is filed as its own story: it also interacts with the handshake credential fields and with how the server maps a certificate subject to a user.

Cloud tests

New Tests/Cloud, [Category("Cloud")], driven by one environment variable holding a native connection string. It ignores itself when unset, and fails if that string does not set UseTls — otherwise the suite would keep passing over a plaintext connection, proving nothing about the transport it exists to cover. Own namespace, so the Integration SetUpFixture does not start a container for it.

Deliberately a smoke set (handshake + ping, query, MergeTree insert round-trip, pooled session reuse, concurrency) rather than the whole integration suite: 50 of that suite's 51 tables are Memory engine, which lives on one replica, while a pooled client spreads an insert and its read-back over connections that need not land on the same one. Widening it means converting those tables or pinning MaxPoolSize=1 — filed separately.

CI: new tests-cloud-tcp.yml, called from tests-tcp.yml on push and manual dispatch only, not on pull_request — every stacked tcp/** epic PR would otherwise queue another run against the shared Cloud service. It carries the same fork guard as the HTTP cloud workflow. reusable-tcp.yml now filters Category!=Cloud.

Testing

TlsParametersTests drives real TLS handshakes against an in-process SslStream server, covering what no cloud service can be asked to present: a self-signed certificate, one from a private authority, one for the wrong purpose, and a chain through an intermediate. TcpConnectionFactoryTests covers the wiring UseTls → BuildTlsParameters → ConnectAsync against a TLS listener that speaks the server Hello, plus the negative that a plaintext client fails there — without that test nothing on the standard CI matrix touched the wiring, only the Cloud job.

Each fix was mutation-checked: dropping the EKU, hardcoding NoCheck, dropping the ExtraStore loop, and inverting the UseTls test each fail exactly their own test.

Verified end-to-end against a ClickHouse container with tcp_port_secure and a self-signed certificate — pinned-CA accept, wrong-CA reject, no-CA reject (UntrustedRoot), skip-verify accept — and the session-reuse test confirmed load-bearing by forcing the pool to retire between operations.

Full suite: 2652 passed, 0 failed on net9.0. Coverage on the new TlsParameters.cs is 96.4%; the only uncovered lines are a documented unreachable defensive guard.

No CHANGELOG/RELEASENOTES entry. This is a deliberate stack-level exception, not an oversight: neither file mentions ClickHouse.Driver.Tcp at all today, the assembly is IsPackable=false and gated behind an experimental diagnostic, and an earlier commit on this stack (07c59bd4) explicitly removed an entry to keep these PRs to the TCP projects. Epic R3 adds the entries as one piece when the client ships. Raised in review and confirmed as the intended behaviour for this stack, so it is settled rather than outstanding. R3 is scoped to sweep every epic, not just the last one.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Adds TLS support to the experimental native TCP client, including certificate validation, secure-port resolution, and Cloud smoke coverage.

Changes:

• Wraps TCP connections in SslStream before protocol authentication.
• Adds TLS options, connection-string keys, validation, and custom CA support.
• Adds TLS unit, integration, Cloud, and CI coverage.

Reviewed changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
ClickHouse.Driver.Tcp/Protocol/TlsParameters.cs	Implements TLS setup and certificate validation.
ClickHouse.Driver.Tcp/Protocol/ClickHouseTcpConnection.cs	Negotiates TLS before protocol handshake.
ClickHouse.Driver.Tcp/Client/IConnectionFactory.cs	Builds and applies TLS configuration.
ClickHouse.Driver.Tcp/Client/ClickHouseTcpConnectionStringBuilder.cs	Adds TLS connection-string keys.
ClickHouse.Driver.Tcp/Client/ClickHouseTcpClientOptions.cs	Defines and validates TLS options.
ClickHouse.Driver.Tcp.Tests/Utilities/TestCertificates.cs	Generates test certificates and chains.
ClickHouse.Driver.Tcp.Tests/Protocol/TlsParametersTests.cs	Tests TLS validation behavior.
ClickHouse.Driver.Tcp.Tests/Protocol/ClickHouseTcpConnectionTests.cs	Updates connection calls for TLS parameters.
ClickHouse.Driver.Tcp.Tests/Integration/TcpServerFixture.cs	Preserves plaintext fixture connections.
ClickHouse.Driver.Tcp.Tests/Integration/ClickHouseTcpConnectionIntegrationTests.cs	Updates integration connection invocation.
ClickHouse.Driver.Tcp.Tests/Cloud/TcpCloudIntegrationTests.cs	Adds Cloud TLS smoke tests.
ClickHouse.Driver.Tcp.Tests/Cloud/TcpCloudFixture.cs	Configures Cloud TLS tests.
ClickHouse.Driver.Tcp.Tests/Client/TcpConnectionFactoryTests.cs	Tests end-to-end TLS factory wiring.
ClickHouse.Driver.Tcp.Tests/Client/ClickHouseTcpConnectionStringBuilderTests.cs	Tests TLS option parsing.
ClickHouse.Driver.Tcp.Tests/Client/ClickHouseTcpClientOptionsTests.cs	Tests TLS defaults and validation.
.github/workflows/tests-tcp.yml	Invokes the TCP Cloud suite.
.github/workflows/tests-cloud-tcp.yml	Defines TCP Cloud testing.
.github/workflows/reusable-tcp.yml	Excludes Cloud tests from standard runs.

Suppressed comments (1)

ClickHouse.Driver.Tcp.Tests/Protocol/TlsParametersTests.cs:320

• The echo server also assumes one read receives the whole payload. A short read gets echoed as a truncated response, making the TLS tests flaky and preventing the client-side exact-read fix from completing. Limit the target memory to the payload length and fill it before echoing.

        int read = await ssl.ReadAsync(buffer);
        await ssl.WriteAsync(buffer.AsMemory(0, read));

---

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[alex-clickhouse]

Cloud job verified by dispatch

The Cloud (TCP) job is gated off pull_request by design, so it never runs on this PR. Dispatched it manually on this branch to confirm it works before merge — run 32002107686, 10/10 jobs green.

The Cloud job itself: 5 passed, 0 skipped, 0 ignored. Worth stating explicitly, because the fixture calls Assert.Ignore when its environment variable is unset — a green job could otherwise have meant five silent skips.

Passed InsertAsync_MergeTreeTable_RoundTripsThroughSelect                   [958 ms]
Passed PingAsync_OverTls_CompletesTheHandshakeAndReportsTheServerIdentity    [388 ms]
Passed QueryAsync_ConcurrentQueriesOverTls_EachReturnItsOwnResultUncrossed   [443 ms]
Passed QueryAsync_OverTls_ReturnsEveryRow                                   [400 ms]
Passed QueryAsync_SequentialQueriesOnOneClient_ReuseThePooledTlsConnection   [393 ms]

This run is the only coverage of four things no local test can reach:

• a publicly trusted certificate validating through the host trust store — every local test uses a private CA or skip-verify, so the default path most users hit was otherwise untested;
• ResolvedPort deriving 9440 with no Port key in the connection string at all;
• the connection string assembled from secrets in YAML, including the password quoting added after review;
• connection affinity through Cloud's load balancer — the temporary-table reuse test only passes if a pooled connection returns to the same replica, which is the assumption the MaxPoolSize = 1 case rests on.

It also confirms the workflow_dispatch clause added to the job condition: without it the job would have been skipped on a manual run, which is what the copied-from-HTTP condition did.

Follow-up, not in this PR: tests-cloud-tcp.yml is workflow_call-only, and a workflow has to exist on the default branch to be dispatchable, so re-running just the Cloud set today means dispatching tests-tcp.yml and paying for the whole 9-job matrix. Once this merges, giving that workflow its own workflow_dispatch trigger makes the smoke set runnable on demand.

[alex-clickhouse]

Both findings were valid. Fixed in cee1a7ef. Suite: 2695 passed, 0 failed on net9.

Blocker — disposal racing an active handshake

Confirmed, and it contradicts what I claimed on the previous commit ("no dial can be in flight"). That holds only when the drain succeeds. ConnectionPool.cs documents the gap in its own words: "A dial in flight is the one connection disposal cannot otherwise reach. It is not in leased yet, so the abort at the end of the drain does not see it." On the PoolTimeout path TakeAllLeased() finds nothing to abort and factory.Dispose() ran regardless.

I did not take the suggested fix. Rather than ref-count live handshakes, disposal is gated on the drain:

if (drained) { factory.Dispose(); }

Holding every permit is the proof that no dial is running — a checkout keeps its permit across its dial, and no new one can start while all are held. When the drain times out the certificates go back to the finalizer, which is exactly where they were before this call existed. Ref-counting would add concurrency machinery to a configuration object to buy prompt handle release in the pathological case only; that did not seem a good trade. If you disagree, the ref-counting version is straightforward to add on top.

Regression test as requested: DisposeAsync_ADialStillRunningPastTheDrainDeadline_LeavesTheFactoryUndisposed gates a dial open past a 150 ms PoolTimeout. Reverting to the unconditional dispose fails it.

Major — top-level revocation ignored under pinning

Correct, and I half-knew it: that interaction is why my own test used the nested property, but I never fixed the contract, so the intuitive setting stayed dead. NormalizeRevocationMode now carries a value set on CertificateRevocationCheckMode into the policy's RevocationMode after the hook runs. A value set directly on the policy wins — both start at NoCheck, so either change is detectable, and a caller who reached into the policy meant that policy. Documented on ConfigureTls.

Two tests, and removing the normalization fails both. Note the nested-wins case cannot be proved by handshake outcome — Offline also rejects a certificate naming no CRL (OfflineRevocation), which my first attempt got wrong — so it asserts the resulting RevocationMode on the policy instance the hook captured.

Nits

• EKU wording: a factual error on my part, fixed. "must declare" → "must not exclude", with the RFC 5280 §4.2.1.12 point that an absent extension is unrestricted and that ApplicationPolicy constrains the extension when present rather than requiring it.
• ClientHello: disambiguated in the TLS files to "the native protocol's client Hello", noting it is not the TLS handshake message of the same name. Not renamed globally — native-protocol.md and ClientHandshakeParameters use ClientHello throughout, so that is established terminology; only the TLS-adjacent comments were ambiguous.
• using var → await using var in the PR description.
• Changelog: kept deferred, but the description now states it as an explicit stack-level exception with the reasoning rather than a bare pointer to R3. Happy to add entries here instead if that is preferred — that one is a maintainer call.

One thing worth recording on the compliance table: the disposal failure mode was fail-closed. A disposed certificate in CustomTrustStore throws rather than validating, so no bad certificate was ever accepted. It was error quality and resource lifetime, not a validation hole — which does not change that it needed fixing.

[alex-clickhouse]

Two decisions from the maintainer, recorded so neither gets re-raised:

No Port == 9440 + UseTls=false guard. It would catch a real footgun — that combination sends the native client Hello, password included, in the clear before the server drops it. But it also breaks a plaintext server legitimately listening on 9440, with no override to escape it, and the server already rejects the connection. Failing a working configuration to catch a misconfiguration is the wrong trade here. Closed rather than deferred; not to be re-raised without an override mechanism alongside it.

No CHANGELOG/RELEASENOTES entry. Confirmed as intended for this stack, not an oversight. Epic R3 is scoped to sweep every TCP epic and write the client up as one piece when it ships — worth knowing that R3 cannot be driven from a changelog diff, since neither file mentions ClickHouse.Driver.Tcp at all today.

Nothing outstanding on my side. B9 is complete: the transport, the option surface, the Cloud smoke suite verified by manual dispatch, and the three review rounds addressed.

[Copilot]

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.

Suppressed comments (1)

ClickHouse.Driver.Tcp/Client/ClickHouseTcpClientOptions.cs:127

• The documented precedence is not true when the callback explicitly sets the policy to NoCheck while setting the top-level mode to Online or Offline. NormalizeRevocationMode cannot distinguish that assignment from the policy's initial NoCheck, so it overwrites the nested value. Document the actual rule so callers do not rely on a precedence the API cannot provide.

    /// With <see cref="TlsCaCertificatePath"/> set, a <c>CertificateChainPolicy</c> is already in place, and the
    /// hook receives it and may edit it. Revocation checking can be asked for either way — on
    /// <c>CertificateRevocationCheckMode</c> or on the policy's own <c>RevocationMode</c> — because a value set on
    /// the former is carried into the latter, which is the only one the handshake reads once a policy exists. A
    /// value set directly on the policy takes precedence.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!