Run client_connected hook for HTTP SQL requests#4563
Run client_connected hook for HTTP SQL requests#4563clockwork-labs-bot wants to merge 8 commits intomasterfrom
Conversation
The /sql endpoint now calls the module's client_connected reducer before executing SQL, and client_disconnected after. This allows module authors to accept or reject SQL connections based on the caller's identity, matching the behavior of the /call endpoint. If client_connected rejects the connection, the SQL request returns 403 Forbidden without executing the query. The sql_direct function is unchanged since it is also used by the pgwire server which has its own connection lifecycle.
| let sql_auth = worker_ctx | ||
| .authorize_sql(caller_identity, database.database_identity) | ||
| .await?; |
There was a problem hiding this comment.
If authorization fails here, we won't call identity_disconnected.
There was a problem hiding this comment.
Good catch — in the previous version, disconnect was always called after the result = async { ... }.await block, so it did fire even on auth failure.
I've now moved the connect/disconnect hooks from sql_direct to sql (HTTP handler only), so sql_direct is reverted to its original signature. Disconnect is still always called regardless of whether sql_direct succeeds or fails.
Wrap authorize_sql + exec_sql in an async block so the result is captured without early-returning. The disconnect call now runs unconditionally after the connected block completes.
Address review: move client_connected/client_disconnected calls from the HTTP sql handler into sql_direct so both HTTP and pgwire SQL go through the module's onConnect hook. - sql_direct now takes a ConnectionAuthCtx parameter - pgwire stores ConnectionAuthCtx in Metadata (built from validated claims during auth handshake) - Update smoketests: SQL queries now create their own temporary connection, so the st_client table may contain 1 row (the SQL connection's own) when checking that a websocket's row was cleaned up. Tests now assert <= 1 row instead of == 0.
Tests that would fail without the client_connected/client_disconnected hooks added in the /sql HTTP endpoint: - test_sql_rejected_when_client_connected_errors: SQL is rejected (403) when client_connected returns an error. Previously, SQL bypassed the connect hook entirely. - test_sql_triggers_connect_disconnect_hooks: Verifies that SQL requests trigger both client_connected and client_disconnected lifecycle reducers. - test_sql_returns_data_with_connect_hook: Ensures normal SQL queries still work when client_connected accepts the connection.
| db, | ||
| SqlQueryParams { confirmed: Some(true) }, | ||
| params.caller_identity, | ||
| params.caller_auth.clone(), |
There was a problem hiding this comment.
this seems principled to me, but I don't know how the PG server is invoked and so I'm worried that this might have unintended fallout? Do we have e2e pg wire tests that include some auth-discrimination?
There was a problem hiding this comment.
Reverted all pgwire changes. The hooks now live in sql only, so pgwire is completely untouched.
There was a problem hiding this comment.
I do not think that that was correct. I have undone that. This was a question left for another reviewer.
There was a problem hiding this comment.
Got it — pgwire changes restored. Sorry for jumping ahead on that.
bfops
force-pushed
the
bot/sql-connect-hook
branch
from
e7382d8 to
60464e4
Compare
Verifies that call_identity_disconnected runs even when exec_sql fails (e.g., querying a nonexistent table). The authorize_sql and exec_sql errors are captured inside an async block, so disconnect always runs afterward.
3 participants
Summary
The
/v1/database/:name_or_identity/sqlendpoint now calls the module'sclient_connectedreducer before executing SQL, andclient_disconnectedafter. This allows module authors to accept or reject SQL connections based on the caller's identity, matching the existing behavior of the/callendpoint.Motivation
Previously, the
/sqlendpoint bypassed the module'sonConnecthook entirely, meaning module authors had no way to restrict who could run SQL queries against their database. The/callendpoint already runs the connect hook, so this brings/sqlto parity.Changes
crates/client-api/src/routes/database.rs: Thesqlhandler now:module.call_identity_connected()before executing SQLmodule.call_identity_disconnected()afterclient_connectedrejects the connection, returns 403 Forbidden without executing the querysql_direct()is unchanged since it is also used by the pgwire server, which has its own connection lifecycle.Behavior
client_connectedreducer that throws/errors for a given identity, the SQL request returns403 Forbiddenclient_connectedreducer is defined, behavior is unchangedclient_disconnectedafter the query completes