feat: Add connection extension framework with symmetric client-server hooks - #316
feat: Add connection extension framework with symmetric client-server hooks#316jiangliu wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a symmetric connection extension framework to ttrpc’s async client/server stack, enabling applications to attach per-connection metadata and optionally transform payload bytes (e.g., for encryption/auth) via hooks and a PayloadTransform.
Changes:
- Introduces a new
extensionmodule with hook/transform traits, connection context, and typed connection metadata propagation. - Wires the connection context through async server/client/stream pipelines, applying transforms at defined injection points and adding a
skip_transforminternal flag for synthetic stream DATA. - Extends async transport
Socketto capture Unixraw_fdso hooks can inspect peers and perform handshake I/O.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/hook_integration.rs | Adds end-to-end integration tests for hooks/transforms over Unix sockets. |
| src/extension.rs | New core extension framework types (hooks, transform trait, connection context) plus unit tests. |
| src/lib.rs | Exposes the new extension module and re-exports key extension APIs. |
| src/proto.rs | Adds non-wire internal_flags to GenMessage and helper constructors for DATA/RESPONSE/close. |
| src/asynchronous/utils.rs | Adds connection_data to TtrpcContext for handler access. |
| src/asynchronous/server.rs | Installs accept hook, propagates connection context, and applies transforms in server pipeline. |
| src/asynchronous/client.rs | Adds Client::with_hook, runs connect hook, and applies transforms in client pipeline. |
| src/asynchronous/stream.rs | Applies transforms for streaming DATA send/recv with skip-transform support. |
| src/asynchronous/transport/mod.rs | Refactors Socket to store Unix raw_fd and exposes as_raw_fd(). |
| src/asynchronous/transport/unix.rs | Ensures accepted/connected Unix sockets capture raw_fd. |
| src/asynchronous/transport/tcp.rs | Ensures accepted/connected TCP sockets capture raw_fd. |
| src/asynchronous/transport/vsock.rs | Ensures accepted/connected vsock sockets capture raw_fd. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
e1496d5 to
8ebbffb
Compare
ce6ccfd to
cf31d6b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
src/asynchronous/server.rs:692
respond()appliestransform_outbound, but it doesn't enforceMESSAGE_LENGTH_MAXon the transformed payload. A transform that expands data can cause oversized responses to be sent (especially for error/status responses that go throughrespond_with_status). Addcheck_oversizeafter the transform.
let payload = self
.conn_ctx
.transform_outbound(payload)
.map_err(|e| Error::Others(format!("transform_outbound failed: {}", e)))?;
let msg = GenMessage::new_response(stream_id, payload);
cf31d6b to
0194e89
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/asynchronous/server.rs:694
respond()appliestransform_outboundbut never enforcesMESSAGE_LENGTH_MAX. Other outbound paths (e.g. the normal unary response path) check size after transform; without a check here, an oversized (or expansion-prone) transform can cause the peer to reject/discard the message and the caller to hang until timeout.
let payload = self
.conn_ctx
.transform_outbound(payload)
.map_err(|e| Error::Others(format!("transform_outbound failed: {}", e)))?;
let msg = GenMessage::new_response(stream_id, payload);
0194e89 to
14655b1
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/asynchronous/server.rs:694
respond()appliestransform_outboundbut does not enforceMESSAGE_LENGTH_MAXafter the transform. A transform (e.g., encryption) can expand the payload beyond the limit, leading to oversized frames being written and then rejected by the peer during read/oversize checks.
let payload = self
.conn_ctx
.transform_outbound(payload)
.map_err(|e| Error::Others(format!("transform_outbound failed: {}", e)))?;
let msg = GenMessage::new_response(stream_id, payload);
14655b1 to
1f6a055
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
src/asynchronous/server.rs:695
respond()appliestransform_outboundbut does not re-check the transformed payload size. A transform can expand data, so this can violateMESSAGE_LENGTH_MAXand produce frames the peer will reject or that can break framing.
let payload = self
.conn_ctx
.transform_outbound(payload)
.map_err(|e| Error::Others(format!("transform_outbound failed: {}", e)))?;
let msg = GenMessage::new_response(stream_id, payload);
src/asynchronous/server.rs:455
- When
transform_outboundfails, this path tries to send an INTERNAL status viarespond_with_status(), which will calltransform_outboundagain and likely fail the same way—leaving the client waiting until timeout. Consider closing the stream/connection to unblock the client when the transform is unusable.
self.respond_with_status(
stream_id,
get_status(Code::INTERNAL, format!("transform_outbound: {}", e)),
)
.await;
1f6a055 to
0325c7b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/asynchronous/server.rs:695
respond()appliestransform_outbound, but it does not enforceMESSAGE_LENGTH_MAX(or callGenMessage::check()) after the transform. A transform can expand payload size (e.g., AEAD tag, framing, compression expansion), which would cause the peer to reject/discard the message and potentially hang waiting for a response. Add a post-transform size check before sending.
let payload = self
.conn_ctx
.transform_outbound(payload)
.map_err(|e| Error::Others(format!("transform_outbound failed: {}", e)))?;
let msg = GenMessage::new_response(stream_id, payload);
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/asynchronous/server.rs:676
respond()now returns an error on oversized responses (viacheck_oversize(...)), which causes the caller to fall back to an INTERNAL status. Previously, oversized responses were converted into an errorResponseand sent back to the client with an appropriate status. This is a behavioral regression and can also interact badly with transforms (the INTERNAL fallback may fail for the same reason). Handle oversize inrespond()by converting the oversize error into aResponseand sending that instead of returning Err.
check_oversize(payload.len(), true)?;
let msg = GenMessage::new_response(stream_id, payload);
self.tx
src/asynchronous/transport/unix.rs:50
- Doc comment refers to a non-existent/incorrect feature name (
security). The code is gated by thesecurity_extensionfeature, so the comment should match the actual feature flag to avoid confusion for readers.
/// Convert a `UnixListener` into a `Listener`.
///
/// Uses `Box::pin(stream! {...})` directly (bypassing `Listener::new()`)
/// so each accepted connection goes through `Socket::from(socket)`,
/// which captures the raw fd when the `security` feature is enabled.
src/asynchronous/transport/tcp.rs:50
- Doc comment refers to a non-existent/incorrect feature name (
security). The code is gated by thesecurity_extensionfeature, so the comment should match the actual feature flag to avoid confusion for readers.
/// Convert a `TcpListener` into a `Listener`.
///
/// Uses `Box::pin(stream! {...})` directly (bypassing `Listener::new()`)
/// so each accepted connection goes through `Socket::from(socket)`,
/// which captures the raw fd when the `security` feature is enabled.
src/asynchronous/transport/vsock.rs:35
- Doc comment refers to a non-existent/incorrect feature name (
security). The code is gated by thesecurity_extensionfeature, so the comment should match the actual feature flag to avoid confusion for readers.
/// Convert a `VsockListener` into a `Listener`.
///
/// Uses `Box::pin(stream! {...})` directly (bypassing `Listener::new()`)
/// so each accepted connection goes through `Socket::from(socket)`,
/// which captures the raw fd when the `security` feature is enabled.
Hi @wllenyj,
|
Tim-Zhang
left a comment
There was a problem hiding this comment.
Thanks for this work. I ran the full async, sync, and security extension tests, clippy, and the main feature combinations. They pass.
I found two design blockers and several bugs around header protection, blocking hooks, size checks, error responses, fd ownership, and existing error behavior. Please address the inline comments before merging.
I also added a few refactor suggestions to keep the sync and async paths consistent and easier to maintain.
513db3c to
89c4c7c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/asynchronous/server.rs:463
- The DATA close-message validation rejects any close frame that carries a payload. With
security_extensionenabled,StreamSender::close_send()appliestransform_outboundto close messages, which can legitimately produce a non-empty wire payload (e.g., AEAD tag/nonce) that decrypts to empty. This check will incorrectly fail encrypted close frames before they reachStreamReceiver::recv()for decryption.
if (msg.header.flags & FLAG_REMOTE_CLOSED) == FLAG_REMOTE_CLOSED
&& !msg.payload.is_empty()
{
src/asynchronous/stream.rs:643
StreamReceiver::recv()returnsEofforREMOTE_CLOSED|NO_DATAwithout verifying that the decrypted payload is empty. If you relax the server-side raw-payload check to allow encrypted close frames, this receiver should enforce the protocol invariant aftertransform_inboundto prevent a peer from smuggling data on a close frame.
let msg = self.transform_data_msg(stream_msg)?;
if (msg.header.flags & FLAG_REMOTE_CLOSED) == FLAG_REMOTE_CLOSED {
self.remote_closed = true;
if (msg.header.flags & FLAG_NO_DATA) == FLAG_NO_DATA {
return Err(Error::Eof);
src/asynchronous/server.rs:679
- On outbound transform failure,
respond()builds a fallbackResponsebut sends it without applying the connection's payload transform. On an encrypted connection, the client will try to decrypt this plaintext fallback and fail to decode the response. The fallback should go throughconn_ctx.outbound()as well (and if that fails, bubble the error so the connection can be closed / handled).
if let Err(e) = self.conn_ctx.outbound(&mut msg, true) {
let fallback = Response::from(e);
let fallback_payload = fallback
.encode()
.map_err(err_to_others_err!(e, "Encode fallback Response failed."))?;
src/asynchronous/server.rs:466
- Typo in the close-message error string: "connot" should be "cannot".
get_status(
89c4c7c to
72ba268
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
src/asynchronous/server.rs:189
- The accept loop awaits
server_ext.on_accept(&conn).awaitinline, so a slow/blocked hook will pauseincoming.next()polling and prevent the server from accepting additional connections until the hook completes (or times out). This contradicts the stated goal that hooks should not stall acceptance, and can materially reduce connection throughput under load. Consider spawning a per-connection task that runs the hook and then callsspawn_connection_handler, so the main accept loop can continue accepting new connections immediately.
Ok(conn) => {
// ── Injection Point 1/10: accept hook ──
#[cfg(feature = "security_extension")]
let conn_ctx = match server_ext.on_accept(&conn).await {
Ok(output) => Arc::new(ConnectionContext::new(output)),
Add the connection extension framework with always-compiled core abstractions and feature-gated hook APIs. Module structure (src/security_extension.rs): - Two mutually exclusive `mod hooks` blocks (one per feature state). Re-export of ConnectionContext/ConnectionData needs no cfg gate. - Always compiled at module top level: ConnectionDataExt trait, PayloadTransform trait, serialize_aad() With security_extension: - ConnectionData = HashMap<String, Box<dyn Any + Send + Sync>> - AcceptHook (server) and ConnectHook (client) traits - HookOutput / HookError / ServerExtensionConfig types - ConnectionContext with real hook-output construction Without security_extension: - ConnectionData is a zero-sized unit struct (0 bytes stack, no heap) - ConnectionContext (noop) with matching public fields (data=Arc<ZST>, payload_transform=None) and size-check-only pipeline methods (inbound, outbound, inbound_buf, outbound_buf) Transport changes (src/asynchronous/transport/): - Socket gains raw_fd field (Unix only, feature-gated) - Socket::from_fd_aware() helper centralizes cfg branching for From<XxxStream> impls across tcp/unix/vsock transport files The framework follows: ttrpc provides mechanism, not policy. Signed-off-by: Jiang Liu <gerry@linux.alibaba.com>
72ba268 to
1fcddb3
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/asynchronous/stream.rs:35
- The
StreamMsg::Wiredoc comment saystransform_inboundhas not been applied yet, butClientReader::handle_msg()decrypts non-DATA messages before sending them into the per-stream channel asStreamMsg::Wire. This makes the comment misleading and can cause future changes to accidentally double-transform (or skip a needed transform) based on incorrect assumptions.
pub(crate) enum StreamMsg {
/// Wire-format message — `transform_inbound` has NOT been applied to the
/// payload yet. `StreamReceiver::recv()` will apply it.
Wire(GenMessage),
Integrate AcceptHook, ConnectHook, and PayloadTransform into the async code path with 10 injection points: Server (Injection Points 1-4): - containerd#1: accept hook on new connection via ServerExtensionConfig - containerd#2: transform_inbound on unary REQUEST with AAD - containerd#3: routing (not transform) for streaming DATA in handle_msg - containerd#4: transform_outbound on unary RESPONSE via respond() Client (Injection Points 5-8): - containerd#5: connect hook on new connection - containerd#6: transform_outbound on unary REQUEST with AAD - containerd#7: transform_inbound on unary RESPONSE with AAD - containerd#8: transform_outbound on stream DATA send Streaming (Injection Points 9-10): - containerd#9: StreamSender::send() applies transform_outbound with AAD - containerd#10: StreamReceiver::recv() applies transform_inbound with AAD using StreamMsg::PreDecoded for already-decrypted initial payloads Cfg gate reduction (~20 gates eliminated): - StreamMsg::PreDecoded, payload_transform fields, transform methods in stream.rs are always compiled (0 gates, down from ~15) - TtrpcContext.connection_data is always present (empty HashMap when feature disabled), removing gates from handler construction - StreamInner::new() always accepts payload_transform parameter (None when no transform configured) Signed-off-by: Jiang Liu <gerry@linux.alibaba.com>
1fcddb3 to
6ed2e5c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 5 comments.
Suppressed comments (2)
src/asynchronous/utils.rs:264
- Adding this field to a publicly constructible context is a source-breaking change: downstream tests or adapters using a
TtrpcContext { ... }literal now fail even whensecurity_extensionis disabled. DerivingDefaultonly helps newly updated literals; it does not preserve existing callers. The connection data should be exposed without changing the required fields of the existing public struct if backward compatibility is required.
/// Opaque per-connection data from [`AcceptHook`](crate::security_extension::AcceptHook). Immutable after accept.
/// See [`ConnectionData`](crate::security_extension::ConnectionData) for full contract.
/// Empty when `security_extension` is not enabled.
pub connection_data: Arc<ConnectionData>,
src/security_extension.rs:429
- The framework timeout is represented as
HookError::Other, even though the public error API has a dedicatedHookError::Timeoutvariant. This prevents logs/monitoring from distinguishing timeouts structurally as promised by the API.
Err(HookError::Other(format!(
"accept hook timed out after {:?}",
ACCEPT_HOOK_TIMEOUT
)))
Integrate AcceptHook, ConnectHook, and PayloadTransform into the sync code path: Sync server: - set_accept_hook() builder method on Server - Invoke hook after listener.accept(); reject closes connection - Propagate ConnectionData and PayloadTransform to connection handler - Apply transform_inbound in reader thread with AAD on received payloads - Apply transform_outbound via send_response() with AAD in handlers Sync client: - with_hook() constructor with ConnectHook - Propagate PayloadTransform from hook output with AAD on transforms Sync utils: - Add connection_data and payload_transform fields to TtrpcContext - Add send_response(): encode + optional transform with AAD + oversize check + send (unified from respond_with_transform + response_to_channel) - Add TtrpcContext::respond() convenience method - Update request_handler! macro to use ctx.respond() Uses ConnectionContext::default() for the feature-disabled path, matching the unified API from the infrastructure commit. Signed-off-by: Jiang Liu <gerry@linux.alibaba.com>
Async integration tests (tests/hook_integration_async_unix.rs): - Plaintext passthrough without hooks - AcceptHook + ConnectHook with XOR PayloadTransform roundtrip (AAD passed through, ignored by XOR) - Server-initiated stream close - Multiple concurrent connections and streams - Client timeout handling - ConnectionData propagation to handler Sync integration tests (tests/hook_integration_sync_unix.rs): - Plaintext passthrough without hooks - AcceptHook called with XOR transform roundtrip - AcceptHook rejection closes connection - ConnectHook rejection fails client construction - XOR transform roundtrip with various payload sizes - ConnectionData propagation to method handler Shared test utilities (tests/common/mod.rs): - XorPayloadTransform with AAD parameter (ignored) for symmetric encrypt/decrypt testing - temp_unix_socket_path() for unique socket path generation Signed-off-by: Jiang Liu <gerry@linux.alibaba.com>
- Add dedicated security-extension job on ubuntu-latest Runs cargo test with --features "async,sync,security_extension" covering 68 unit tests + 19 async IT + 6 sync IT - Make Makefile feature flags platform-aware The security_extension feature is Unix-only (compile_error! on other platforms), so test/check targets use --all-features on Unix and --features sync,async on Windows. Sub-crate Makefiles (compiler, ttrpc-codegen) pre-set FEATURES=--all-features since they don't have these features. Signed-off-by: Jiang Liu <gerry@linux.alibaba.com>
6ed2e5c to
7f40c7b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.
Suppressed comments (5)
src/asynchronous/stream.rs:579
- Unlike
send(), this transformed close-frame path never performs the post-transform size check. A transform that expands an empty payload beyondMESSAGE_LENGTH_MAXis therefore queued with an oversized frame instead of returning an error. Check the message after transformation before sending it.
let mut msg = GenMessage::new_close(self.stream_id);
// ── Close-message transform: authenticate header via AAD ──
self.transform_msg(&mut msg)?;
_send(&self.tx, msg).await?;
src/security_extension.rs:330
- This runtime requirement is incorrect: Tokio supports
spawn_blockingon a current-thread runtime and executes the closure on an additional blocking thread. Keeping this warning unnecessarily excludes a supported deployment and contradicts the preceding statement that current-thread runtimes are handled safely.
/// **Requirement**: the async server must run on a **multi-thread** tokio
/// runtime; `spawn_blocking` panics on current-thread runtimes.
src/asynchronous/transport/mod.rs:118
- The captured descriptor belongs to a Tokio stream and remains nonblocking. Consequently, the synchronous hooks' documented handshake reads can return
WouldBlockbefore peer data arrives, andSO_RCVTIMEOdoes not make I/O wait on anO_NONBLOCKdescriptor. Provide readiness-aware/async hook I/O, or put the descriptor into blocking mode exclusively for the offloaded hook and restore nonblocking mode before starting Tokio I/O; a multi-step handshake integration test should exercise this race.
let fd = socket.as_raw_fd();
Self::with_raw_fd(socket, fd)
src/security_extension.rs:429
- The framework-generated timeout is returned as
HookError::Other, so callers cannot distinguish it using the dedicatedHookError::Timeoutvariant promised by this API. Return the structured timeout variant here.
Err(HookError::Other(format!(
"accept hook timed out after {:?}",
ACCEPT_HOOK_TIMEOUT
)))
src/asynchronous/server.rs:462
- The changed Rust sources are not rustfmt-clean (this spacing is one example, along with long unformatted signatures in the new transport and extension code). The
checkCI job runscargo fmt --all -- --check, so this PR will fail that required job untilcargo fmt --allis applied.
Err(status) => self.respond_with_status( stream_id, status).await,
Summary
ttrpc currently provides no mechanism for applications to inject connection-level policies such as encryption, authentication, or file-descriptor passthrough. Each RPC message passes through serialization and transport layers with no opportunity for the application to inspect or transform the wire-format bytes, leaving security-critical concerns to be handled out-of-band or not at all.
This PR introduces a symmetric extension framework that exposes 10 injection points across the client and server lifecycle, enabling applications to implement composable security policies (e.g., AES-GCM encryption, Ed25519 authentication) without modifying ttrpc core.
Design Principles
AcceptHook(server) andConnectHook(client) share the sameHookOutput/HookErrortypes andPayloadTransformtrait.payload_transform = None→ plaintext pass-through.10 Injection Points
do_start()acceptAcceptHook::on_accepthandle_request()transform_inboundhandle_msg()DATAhandle_method()responsetransform_outboundnew_inner()connectConnectHook::on_connectrequest()sendtransform_outboundhandle_msg()recvtransform_inboundnew_stream()sendtransform_outboundStreamSender::send()transform_outboundStreamReceiver::recv()transform_inboundKey design decision: Streaming DATA messages are routed (not transformed) in
handle_msg()(#3). The transform is applied exclusively inStreamSender::send()/StreamReceiver::recv()(#9/#10) to avoid double-transform.streaming_client=falsewith initial payloadWhen
streaming_client=falseand the stream-init REQUEST carries a payload,handle_stream()creates a synthetic DATA message from the already-decrypted REQUEST payload (decrypted at #2). Apub(crate) internal_flagsfield onGenMessagecarries askip_transformbit soStreamReceiver::recv()(#10) passes it through without re-applyingtransform_inbound. This works for anyPayloadTransform, including asymmetric transforms.New Public API
Server registration
Client registration
Socket raw_fd Capture
Socketnow stores the underlyingRawFd(Unix only) so hooks can callgetpeername()for peer identity inspection (e.g., vsock CID) and perform bidirectional handshake I/O. Platform-specificFromimpls (TcpStream,UnixStream,VsockStream) capture the fd viaas_raw_fd(); the genericSocket::new()setsraw_fd = None.Files Changed
src/extension.rsConnectionContext, 20 unit teststests/hook_integration.rssrc/proto.rsGenMessage::internal_flags+ helper constructorssrc/lib.rsHookError,HookOutput,AcceptHook,ConnectHooksrc/asynchronous/server.rsset_accept_hook(),on_accept()in accept loop, injection points #2/#3/#4src/asynchronous/client.rswith_hook(),ConnectHookinnew_inner(), injection points #6/#7/#8src/asynchronous/stream.rspayload_transformfield on sender/receiver, injection points #9/#10src/asynchronous/transport/mod.rsSocketgainsraw_fd: Option<RawFd>,Socket::from()captures fdsrc/asynchronous/transport/{tcp,unix,vsock}.rsFromimpls captureas_raw_fd()src/asynchronous/utils.rsTtrpcContext::connection_datafieldTest Coverage
87 tests (67 unit + 19 integration + 1 example):
ConnectionContextconstruction, Arc sharing, transform pass-throughstreaming_client=falsewith initial payload (skip_transform path)streaming_server=falseDATA message rejectionBackward Compatibility
payload_transform = Noneconnection_datais empty HashMap — no breakage