fix(sdk-ruby): verify the TLS peer on WebSocket dials - #249
Conversation
…edential The five credential-bearing `wss://` dials in `process.rb` and `code_interpreter.rb` did not verify the peer certificate. `websocket-client-simple` builds its own `SSLContext` and never calls `SSLContext#set_params`, so Ruby's own defaults do not apply; it honours `verify_mode` only when passed, and cannot enable hostname verification at all. Since the context is frozen by `SSLSocket.new` and the handshake is written before `connect` returns, there is no caller-side hook, so route the dials through a shared client that owns the TLS setup. Chain verification alone is insufficient: without a hostname check a peer holding any valid certificate can terminate the connection and read the request headers. Also stop sending the organization credential on these dials. They authenticate on `X-Daytona-Preview-Token`, and the proxy strips `Authorization` before forwarding, so it was only ever exposed on the hop being fixed. Headers are reduced to an allowlist inside the dialer rather than at each call site, so a dial added later cannot opt out and a credential added to `default_headers` in future is excluded by default. The existing specs stub the dial and cannot observe TLS. The new spec stands up a local TLS listener and asserts on the bytes the peer received, covering the wrong-hostname case a `verify_mode`-only change would miss, that the organization credential is absent from the wire even when a caller supplies one, and a send/close round trip that fails if the reimplemented connect stops satisfying the inherited client's contract. A structural guard fails if a dial appears outside the shared client. Pin `websocket-client-simple` to `~> 0.9.0`: the reimplementation couples to instance variables the inherited methods read, and a 0.x minor bump could rename them. Signed-off-by: Ante Projić <aprojic@daytona.io>
Vidoc security reviewTip Good to merge — no security issues found. Reviewed 9 changed files. 💬 Have questions? Tag @vidoc in a comment and I'll answer. |
There was a problem hiding this comment.
All reported issues were addressed across 9 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
…down Only seed the system trust roots into a store the dialer creates itself. Adding them to a store the caller supplied would silently widen their trust policy rather than honour it. In the spec, close the client each dial opens: the reader loop survives the peer going away and would otherwise spin re-raising EOFError for the rest of the run. Also narrow the recording listener's rescue so that reading frames until the peer disconnects is no longer recorded as a handshake failure, which made the captured log misreport why a connection ended. Signed-off-by: Ante Projić <aprojic@daytona.io>
|
On dropping
bearerToken := p.getBearerToken(ctx)
if bearerToken != "" {
isValid, err := p.getSandboxBearerTokenValid(ctx, sandboxIdOrSignedToken, bearerToken)
...
// If authentication successful, remove the Authorization header to prevent it from being forwarded to the sandbox
ctx.Request.Header.Del("Authorization")
}
authKey := ctx.Request.Header.Get(SANDBOX_AUTH_KEY_HEADER)Bearer is attempted first and only falls through to Two asks:
Least privilege is a fair argument on its own terms and I'm not against it. But it's now a separable choice with a cost, so I'd suggest landing the TLS fix now (urgent, no downside) and giving the allowlist its own PR where the preview-token reliability question can be settled first. The rest looks good to me — the |
…hange Withholding the organization credential from these handshakes was motivated by the peer being unverified. With the certificate and hostname now checked before anything is written, sending it to a verified peer is what the rest of the SDK already does, so the TLS change addresses the exposure on its own. Dropping the credential is a least-privilege improvement rather than part of that fix, and it is not free: the proxy attempts the bearer token before the preview token, so today these dials most likely authenticate on the bearer and never reach the preview-token branch. Removing it makes that branch the only path, with no fallback. Whether it behaves acceptably as the sole path - in particular its rate-limiting - should be settled on its own terms rather than as a side effect of a TLS fix. Leaves the dials sending the headers they sent before, over a verified connection. Signed-off-by: Ante Projić <aprojic@daytona.io>
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
What
The Ruby SDK's WebSocket dials did not verify the peer certificate.
websocket-client-simplebuilds its own
SSLContextand never callsSSLContext#set_params, so Ruby's own defaults(
verify_mode: VERIFY_PEER,verify_hostname: true) never apply. It honoursverify_modeonlywhen passed, and has no option for hostname verification at all.
Passing
verify_modeis not sufficient on its own. Without a hostname check, a peer holding anyvalid certificate can terminate the connection and read the request headers. The context is frozen
by
SSLSocket.newand the handshake is written beforeconnectreturns, so there is nocaller-side hook to correct this — the dial itself has to be owned.
Daytona::Common::WebSocketDialernow owns it, and the five dials inprocess.rbandcode_interpreter.rbroute through it.common/socketio_client.rbis unchanged: it alreadyverifies, and sends its token after verification rather than in the handshake.
The dials send the same headers they sent before, now over a verified connection. An earlier
revision of this PR also reduced them to an allowlist; that is deferred to its own change, since
the proxy attempts the bearer token before the preview token and removing it would make the
preview-token branch the only path. Worth settling on its own terms.
Tests
The existing specs stub the dial, so they cannot observe TLS at all. The new spec stands up a
local TLS listener and asserts on the bytes the peer received:
case a
verify_mode-only change passespass merely because the CA is untrusted)
connectstops satisfying theinherited client's contract
No network egress, no fixture files, no new dev dependencies.
Notes for review
#connectis reimplemented rather than extended because upstream builds the socket, performs thehandshake and writes the request in one pass, and returns early when the socket is already set.
Everything after TLS setup mirrors upstream so framing, the reader thread and event semantics stay
identical. That couples to the instance variables the inherited
#send,#close,#open?and#closed?read, sowebsocket-client-simpleis pinned to~> 0.9.0—~> 0.9would admit 0.10,where a 0.x rename could land. The round-trip spec is what catches that contract breaking.
System trust roots are seeded only into a store the dialer creates itself, so a caller-supplied
:cert_storestays authoritative.Coverage gap, stated rather than implied:
get_session_command_logs_asyncandget_entrypoint_logs_asynchave no e2e coverage. Their handshake construction is identical to thePTY dials that do, so confidence transfers by inspection, not by test.
Consumer impact
Connections to a preview endpoint whose certificate does not verify now raise
OpenSSL::SSL::SSLErrorinstead of silently connecting. Daytona-hosted regions are unaffected. Acustom region presenting a privately issued certificate needs its CA in the system trust store —
the dialer reads it. There is deliberately no option to disable verification, matching the Go,
Node, Python and Java SDKs.
Coordination
#191 adds
ProcessHandle#attach_terminal, a further dial. It targets the toolbox API host ratherthan a preview link and carries no preview token. Flagging it so whichever lands second routes the
dial through the shared dialer.