Serve builtin services only on ServerOptions.internal_port - #3525
Serve builtin services only on ServerOptions.internal_port#3525chenBright wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The “internal_port serves builtin/Tabbed only” contract is not enforced consistently across all server-side protocols yet, and one newly introduced message/comment has correctness issues.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR tightens the security model around ServerOptions.internal_port by ensuring that internal-port traffic can’t be used to “authenticate” a connection via builtin endpoints and then reuse that authenticated connection to access ordinary services without credentials.
Changes:
- Add server-level helpers (
Server::RejectBuiltinAccess,Server::RejectNonBuiltinAccessFromInternalPort) and apply them in several protocol handlers to block ordinary services oninternal_port. - Extend unit tests to verify ordinary services are rejected on
internal_port(including pooled-connection latch scenarios) and that Nshead/Thrift-style dispatch paths are also gated. - Update documentation for the revised
internal_portbehavior and error messages; remove the old inlineRejectBuiltinAccesshelper fromserver_private_accessor.h.
File summaries
| File | Description |
|---|---|
| test/brpc_server_unittest.cpp | Adds coverage that ordinary services (PB + HTTP) are rejected on internal_port, plus Nshead-specific gating behavior. |
| test/brpc_http_rpc_protocol_unittest.cpp | Adds a pooled-connection test demonstrating the “builtin request latches connection” scenario is now harmless because only builtin services remain reachable on internal_port. |
| src/brpc/server.h | Documents the stronger internal_port contract and declares new rejection helpers on Server. |
| src/brpc/server.cpp | Implements the new rejection helpers and standardized failure messages. |
| src/brpc/policy/thrift_protocol.cpp | Rejects Thrift requests received on internal_port. |
| src/brpc/policy/sofa_pbrpc_protocol.cpp | Applies both builtin-access rejection (security mode) and non-builtin rejection (internal port) before dispatch. |
| src/brpc/policy/nshead_protocol.cpp | Applies internal-port rejection for nshead-style services (which don’t dispatch via MethodProperty). |
| src/brpc/policy/hulu_pbrpc_protocol.cpp | Applies both builtin-access rejection (security mode) and non-builtin rejection (internal port) before dispatch. |
| src/brpc/policy/http_rpc_protocol.cpp | Applies both builtin-access rejection (security mode) and non-builtin rejection (internal port) for HTTP RPC dispatch. |
| src/brpc/policy/baidu_rpc_protocol.cpp | Applies both builtin-access rejection (security mode) and non-builtin rejection (internal port) before dispatch. |
| src/brpc/nshead_pb_service_adaptor.cpp | Applies the new server-level rejection helpers in the adaptor dispatch path. |
| src/brpc/details/server_private_accessor.h | Removes the old inline RejectBuiltinAccess helper (now centralized on Server). |
| docs/en/server.md | Updates internal_port documentation to reflect builtin-only (and Tabbed) serving and the new rejection error. |
| docs/cn/server.md | Same as English docs update for internal_port behavior and rejection rationale. |
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // Reject accesses to non-builtin services arriving at ServerOptions.internal_port, | ||
| // which is documented as the place to expose builtin services away from the public | ||
| // listener, not as a second entrance to the ordinary services of the server. Serving | ||
| // them there is what makes the authentication exemption of the internal port escape | ||
| // a single request: verify() is only run for the FIRST message of a connection and | ||
| // its verdict latches the whole connection, so an unauthenticated builtin request | ||
| // used to mark the connection as authenticated and every later request on it skipped | ||
| // verification altogether. |
| cntl->SetFailed(EPERM, "Only builtin services are accessible on " | ||
| "ServerOptions.internal_port=%d, send the request to the port " | ||
| "passed to Server::Start() instead", | ||
| _options.internal_port); |
| // Returns true if the access was rejected, in which case `cntl` was already etFailed() | ||
| // and the caller must stop dispatching the request immediately. |
wwbmmm
left a comment
wwbmmm
left a comment
There was a problem hiding this comment.
The change correctly narrows internal_port to builtin/tabbed services and adds tests, but the new RejectNonBuiltinAccessFromInternalPort gate is not applied on every request-dispatch path, so the stated guarantee that only builtin services are served on internal_port does not fully hold.
🤖 This reply was automatically generated by brpc-oncall
| return; | ||
| } | ||
| if (RejectBuiltinAccess(cntl, *server, mp)) { | ||
| if (server->RejectBuiltinAccess(cntl, mp) || |
There was a problem hiding this comment.
The new gate is placed after the server->options().http_master_service early-dispatch block (around line 1543), which calls into the user-provided http_master_service and returns before reaching this check. http_master_service is a non-builtin user service, so on a server that sets both internal_port and http_master_service, an ordinary (non-builtin) service is still reachable on internal_port, contradicting both the new docs and the guarantee that only builtin/tabbed services are served there. The same pattern also exists for baidu_master_service in baidu_rpc_protocol.cpp, whose dispatch happens before the new gate. Please move the internal-port restriction ahead of these master-service dispatch blocks (or apply it centrally) so the gate covers every non-builtin path.
🤖 This reply was automatically generated by brpc-oncall
What problem does this PR solve?
Issue Number: resolve
Problem Summary:
Authentication configured through
ServerOptions.authcan be bypassedentirely on a server that also sets
ServerOptions.internal_port.VerifyHttpRequest()returns true without checking any credential when abuiltin service is requested on
internal_port. That verdict is notper-request:
verify()runs only for the first message of a connectionand the result latches the connection. Sending
GET /statustointernal_portfirst therefore authenticates the connection, and everythingsent next on it is dispatched without ever calling
verify().What is changed and the side effects?
Changed:
internal_portnow carries builtin and Tabbed services only. Requests forordinary services are rejected with
EPERM(HTTP 403) and must go to theport passed to
Server::Start(). With nothing but builtin services servedthere, the latch has nothing left to unlock.
Side effects:
Performance effects:
Breaking backward compatibility:
Check List: