Fix streaming RPC frame validation - #3481
Conversation
There was a problem hiding this comment.
Pull request overview
This PR strengthens the server-side streaming RPC protocol handling in bRPC by ensuring streaming frames are subject to authentication state and by preventing streams from accepting frames over a different socket than the one that established/bound the stream via the creating RPC.
Changes:
- Enforce server-side rejection of streaming frames when server authentication is configured but the socket has not successfully authenticated.
- Bind server-accepted streams to the creating RPC’s socket and drop frames delivered on “foreign” sockets.
- Add unit tests covering unauthenticated streaming-frame rejection and end-to-end authenticated RPC + streaming behavior (including using an OS-assigned port to avoid conflicts).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/brpc_streaming_rpc_unittest.cpp | Adds unit tests for unauthenticated-frame rejection and authenticated service-path streaming. |
| src/brpc/stream.cpp | Tightens socket binding behavior for streams; rejects frames from mismatched sockets. |
| src/brpc/stream_impl.h | Tracks whether a stream is server-accepted to enforce socket-binding rules. |
| src/brpc/socket.h | Exposes a new Socket::IsAuthenticated() API for protocol-layer checks. |
| src/brpc/socket.cpp | Implements Socket::IsAuthenticated() using the existing auth flag/error state. |
| src/brpc/policy/streaming_rpc_protocol.cpp | Adds an authentication-state gate for server-side streaming frame parsing (when invoked with server context). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (_host_socket != nullptr) { | ||
| return 0; | ||
| return _host_socket->id() == host_socket->id() ? 0 : -1; | ||
| } |
There was a problem hiding this comment.
Updated all existing SetHostSocket callers to handle binding failures. The server response paths now fail the affected streams instead of marking them connected, and the client extra-stream path fails the request streams and returns.
| brpc::Server server; | ||
| server._options.auth = &auth; |
There was a problem hiding this comment.
Updated the test to configure authentication through public ServerOptions and Start(0, ...). It continues to use an OS-assigned port.
63bc640 to
50ba0db
Compare
What problem does this PR solve?
Issue Number: resolve
Problem Summary:
Streaming RPC frames are consumed directly by the streaming parser and do not pass through
InputMessenger's authentication hook. A stream could also accept a frame carrying its ID from a socket other than the socket that created the stream.What is changed and the side effects?
Changed:
Side effects:
Performance effects: One authentication-state check per server-side streaming frame when server authentication is configured, plus a socket identity check before dispatching a frame to a stream.
Breaking backward compatibility: No. Authenticated streaming RPC behavior and the wire format are unchanged.
Check List: