Serve builtin services only on ServerOptions.internal_port - #3525
Open
chenBright wants to merge 1 commit into
Open
Serve builtin services only on ServerOptions.internal_port#3525chenBright wants to merge 1 commit into
chenBright wants to merge 1 commit into
Conversation
Contributor
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.
Comment on lines
+637
to
+644
| // 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. |
Comment on lines
+2399
to
+2402
| 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); |
Comment on lines
+645
to
+646
| // Returns true if the access was rejected, in which case `cntl` was already etFailed() | ||
| // and the caller must stop dispatching the request immediately. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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: