Conversation
Serve ClickHouse's native TCP protocol alongside the HTTP interface, so clickhouse-client, clickhouse-driver and clickhouse-go can be used with a PAM account, and so a server with HTTP disabled can be used at all. A session hands out one local port and routes each connection by its first byte, so the driver decides the protocol rather than the user. The native handler swaps the client's credentials for the account's during the handshake, then parses every client packet: the statement arrives in its own length-prefixed Query packet, which is matched against the command blocking policy and written to the session recording. Data blocks are decoded only far enough to find where they end and are forwarded byte for byte, because re-encoding them would mean reproducing a serialization we do not own. A packet the loop cannot read ends the session rather than being relayed uninspected. For a server with no HTTP interface, the gateway answers HTTP itself by running the statement over the native protocol. ClickHouse formats the values through formatRow, so every column type keeps working without the gateway decoding one. That path exists for Web Access, which is HTTP-only; a third-party HTTP client is turned away with an explanation. An account now carries an HTTP port, a native port, or both, with at least one required. The connection test probes each port that is set and names the one that failed, and a gateway that does not report native support refuses to save an account with a native port rather than failing at session time.
|
💬 Discussion in Slack: #pr-review-cli-414-feat-clickhouse-native-protocol-support Posted by Review Police — reviews, comments, new commits, and CI failures will stream into this channel. |
11 tasks
Contributor
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
Contributor
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 3 · PR risk: 0/10 |
This comment has been minimized.
This comment has been minimized.
ch-go allocates a declared string length before reading it, so an unauthenticated client could name a terabyte and take the process down. Handshake fields are now read through a bounded reader. The session revision was pinned to min(client, ch-go) and never clamped to the upstream's. Against a server older than ch-go that put feature-gated bytes on the wire it never reads, desynchronising the stream. A native port that accepts the connection and then says nothing dropped the deadline error, so it classified as a rejected credential and stopped the heartbeat schedule. It now wraps, and the handshake read is bounded by the probe's own budget rather than outliving it. Also refuses a connection test that was given no port, and classifies an unauthorised port as a transport failure instead of leaving it unknown.
The handler package carried 23 tests behind PAM_CLICKHOUSE_NATIVE_IT that needed an ambient ClickHouse with hand-seeded tables. No fixtures were committed and no CI job ran them, so they only ever ran on one machine. Real-client interop moves to e2e/pam, where testcontainers starts the server and the container's own clickhouse-client drives the session, matching the Postgres and Redis suites. The handler package now runs with no containers and no environment variables, and CI runs it under -race along with gateway-v2. e2e is a separate module and its go.mod had gone stale against the new ch-go dependency, which was failing three CI jobs.
The policy matched the recorded form of a statement, which carries a "-- parameters:" suffix, while ClickHouse received the bare SQL. An end-anchored rule stopped matching the moment a client attached a parameter and the blocked statement ran. Both forms are now checked, on the HTTP path as well as the native one. The e2e test drove clickhouse-client from inside the container, which can never reach a PAM proxy: those bind loopback only, and TestLocalProxiesBindLoopback enforces it. It now drives ch-go's client from the host instead. A handshake bounded by a shorter probe budget also reported the ten-second limit rather than the one it applied.
ch-go allocates a declared string length before it reads a byte and rejects only a length that goes negative, so a client naming a terabyte took the gateway down with it. Verified in a 512 MB container: 4 GB allocates fine because untouched pages never become resident, while 1 TB is SIGKILL, which no recover can catch, and it takes every other session on that gateway. The query packet is now decoded field for field with every string read through a cap, and the settings and parameters lists carry a count bound so neither can grow without limit. A round-trip test encodes with ch-go and decodes with ours across three revisions, since a field read in the wrong order would desynchronise the stream, which is the failure this exists to prevent. The handshake was already bounded; those helpers move alongside the rest.
The ClickHouse ports start listening before the entrypoint has created CLICKHOUSE_DB, so seeding raced initialisation and clickhouse-client exited 81, UNKNOWN_DATABASE. The wait now runs a query against the database itself, which is what proves it is usable. The HTTP helper also asserted on its request, and the wait for the proxy to bind called it in a retry loop, so a connection refused while the port was coming up would have failed the test instead of retrying.
The PAM e2e job checks out infisical/main with no ref, so a CLI test can only exercise backend behaviour that has already merged. Against main the account's nativePort is stripped as an unknown key and port is still required, so every ClickHouse account comes back HTTP-only and three of the four subtests fail on a backend that predates the feature. Every other PAM e2e test landed this way, in its own PR once the backend was in place: redis shipped in December and its test followed in April. This one follows the same route. The go.mod tidy stays, since that failure is real and independent.
… races Per-field caps only bound one field at a time, so thousands of individually legal settings still added up. The query packet now carries one budget across every field it decodes. The refusal check sat outside the lock that orders writes to the client, so a server packet cleared a moment before a refusal could still land after the exception. The check now happens under that lock. The server loop returning left the client blocked on a read until the idle deadline when the upstream went away, waiting for a result that could never arrive. It now ends the session instead.
…anic ch-go sizes a column from the declared row count before reading any of it, so a ~30 byte data block header committed 763 MB for Int64 and 3.2 GB for Int256 while the client sent nothing further. That allocation succeeds rather than panicking, so the handler's recover could not catch it. The block header is now scanned before the decoder sees it, without consuming it, so an absurd row or column count is refused. The scan is best-effort by design: anything it cannot parse falls through to ch-go, so a mistake in it can only miss an attack, never reject real traffic. It peeks only what has already arrived, since a fixed window would stall a session whose next block is smaller than it. Compressed blocks are left alone, being already capped by ch-go. Session teardown was straight-line after the client loop, so a panic there skipped it and every in-flight statement vanished from the session log without even an INTERRUPTED. It is deferred now. A near-exhausted probe budget rendered as "within 0s" and told the operator their port was misconfigured, which is a wrong diagnosis for a timeout the test itself caused. It now says the budget ran out, and the test that locked in the one duration that rendered correctly asserts the meaning instead. The upstream-disconnect test set its read deadline after triggering the teardown it was testing, so it failed about 1% of runs on the setup line rather than the assertion. 300 runs clean.
The header pre-scan could be skipped by a split header or a compressed block. The limits now run in the decoder's result hook, which sees the parsed row and column counts before any column is allocated.
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
1 participant
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.
Description 📣
Serves ClickHouse's native TCP protocol alongside HTTP, so
clickhouse-client,clickhouse-driverandclickhouse-gowork with a PAM account, and a server with HTTP disabled works at all.One local port, routed per connection by its first byte. The handshake swaps the client's credentials for the account's. Statements arrive in their own packet, so blocking and recording apply as they do over HTTP. A packet the gateway can't read ends the session rather than being relayed uninspected.
Nothing translates between the two interfaces. An account with no HTTP port turns HTTP clients away with an explanation, and an account with no native port does the same for native clients.
Type ✨
Tests 🛠️
136 tests across
clickhouseandgateway-v2, no containers and no environment variables. Both packages now run in CI under-race.Real-client interop was verified by hand through a real session:
clickhouse-clientand Python'sclickhouse-driveracross compression, parameters, settings, INSERT and exotic column types, plus native-only, HTTP-only and dual-port accounts. The e2e suite test follows in its own PR once the backend half is on main, the way the other PAM account types were done, since that job runs againstinfisical/main.go test -race ./packages/pam/handlers/clickhouse/ ./packages/gateway-v2/...