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.
wwbmmm
reviewed
Sep 6, 2026
wwbmmm
left a comment
wwbmmm
left a comment
Contributor
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
chenBright
force-pushed
the
fix_internal_port
branch
from
September 7, 2026 09:31
b498dc7 to
6699288
Compare
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.
Comment on lines
+1231
to
+1237
| static int PickUnusedPort() { | ||
| butil::fd_guard sockfd(butil::tcp_listen(butil::EndPoint(butil::IP_ANY, 0))); | ||
| EXPECT_LE(0, sockfd); | ||
| butil::EndPoint point; | ||
| EXPECT_EQ(0, butil::get_local_side(sockfd, &point)); | ||
| return point.port; | ||
| } |
Comment on lines
+1751
to
+1760
| ASSERT_EQ(0, str2endpoint("127.0.0.1:8613", &ep)); | ||
| butil::EndPoint internal_ep; | ||
| ASSERT_EQ(0, str2endpoint("127.0.0.1:8614", &internal_ep)); | ||
|
|
||
| brpc::Server server; | ||
| EchoServiceImpl echo_svc; | ||
| ASSERT_EQ(0, server.AddService(&echo_svc, brpc::SERVER_DOESNT_OWN_SERVICE)); | ||
| brpc::ServerOptions opt; | ||
| opt.internal_port = internal_ep.port; | ||
| ASSERT_EQ(0, server.Start(ep, &opt)); |
|
|
||
| ``` | ||
| [a27eda84bcdeef529a76f22872b78305] Not allowed to access builtin services, try ServerOptions.internal_port=... instead if you're inside internal network | ||
| Not allowed to access builtin services, try ServerOptions.internal_port=... instead if you're inside internal network |
Comment on lines
+205
to
+214
| // Update: this port carries builtin and Tabbed services only, requests | ||
| // for ordinary services are rejected with EPERM and must be sent to the | ||
| // port passed to Start(). Builtin requests are exempted from | ||
| // ServerOptions.auth here and the exemption authenticates the connection | ||
| // they arrive on, hence ordinary services would be reachable without | ||
| // credentials from the same connection. http_master_service and | ||
| // baidu_master_service answer for every URL or service name, they are | ||
| // ignored on this port so that the builtin services behind them stay | ||
| // reachable. redis_service runs its command handlers too early for an | ||
| // EPERM to be sent back and is not served here at all. |
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: