Copilot commented on code in PR #3525:
URL: https://github.com/apache/brpc/pull/3525#discussion_r3951118216
##########
src/brpc/policy/baidu_rpc_protocol.cpp:
##########
@@ -306,10 +312,10 @@ void SendRpcResponse(int64_t correlation_id, Controller*
cntl,
}
cntl->CallAfterRpcResp(req, res);
- if (nullptr == server->options().baidu_master_service) {
- server->options().rpc_pb_message_factory->Return(messages);
- } else {
+ if (IsBaiduMasterService(server, cntl->local_side())) {
BaiduProxyPBMessages::Return(static_cast<BaiduProxyPBMessages*>(messages));
Review Comment:
Returning `messages` to different pools based on `local_side` is only safe
if allocation also uses the same condition. If `baidu_master_service` causes
`messages` to be allocated as `BaiduProxyPBMessages` unconditionally (as the
previous return logic implied), then returning internal-port requests via
`rpc_pb_message_factory->Return(messages)` risks returning an object to the
wrong pool (memory corruption / type confusion). A robust fix is to record the
allocation/dispatch path (e.g., a boolean on the controller/request context)
and use that to decide the return path, or ensure allocation uses
`IsBaiduMasterService(...)` too.
##########
src/brpc/server.h:
##########
@@ -616,6 +619,32 @@ class Server {
// Returns true if accept request, reject request otherwise.
bool AcceptRequest(Controller* cntl) const;
+ // Reject accesses to builtin services when the server is in security mode,
+ // in which case they are only reachable from ServerOptions.internal_port.
+ // Returns true if the access was rejected, in which case `cntl` was
already
+ // SetFailed() and the caller must not let the request reach the service's
+ // normal processing. nshead hands the failed Controller to NsheadService
+ // instead, as it does for the other pre-checks, see nshead_protocol.cpp.
+ // NOTE: Call this after ControllerPrivateAccessor::set_security_mode() and
+ // before the method is counted by MethodStatus::OnRequested(), so that
+ // rejected accesses do not pollute the stats of the method. `mp` may point
+ // to BadMethodService which is builtin as well and lists the methods of
the
+ // requested service, so protocols dispatching to BadMethodService must
call
+ // this beforehand, or make sure the listing is hidden in security mode.
+ bool RejectBuiltinAccess(Controller* cntl, const MethodProperty* mp) const;
+
+ // Reject accesses to non-builtin services arriving at
ServerOptions.internal_port,
+ // which exposes builtin services away from the public listener, it is not
a second
+ // entrance to the ordinary services of the server.
+ // NOTE: Same return contract and placement rules as RejectBuiltinAccess().
+ bool RejectNonBuiltinAccessFromInternalPort(Controller* cntl,
+ const MethodProperty* mp)
const;
+ // This overload is for the protocols dispatching to a service that is
never
+ // builtin (NsheadService, ThriftService), hence has no MethodProperty.
+ bool RejectNonBuiltinAccessFromInternalPort(Controller* cntl) const;
+ // True if `local_side` is ServerOptions.internal_port.
+ bool IsInternalPort(const butil::EndPoint& local_side) const;
Review Comment:
These look like internal dispatch helpers but are added to the public
`brpc::Server` interface (in `server.h`). This increases the public API surface
with methods that are difficult to support as stable API. Consider keeping them
in an internal header (e.g., `details/`) or routing access via
`ServerPrivateAccessor`/friend declarations so protocol implementations can use
them without making them public.
##########
test/brpc_server_unittest.cpp:
##########
@@ -1719,6 +1721,158 @@ TEST_F(ServerTest,
builtin_services_are_gated_by_internal_port) {
ASSERT_EQ(0, server.Join());
}
+// Call the same ordinary service the way a browser would.
+void CallEchoByHttp(const butil::EndPoint& ep, brpc::Controller* cntl) {
+ brpc::ChannelOptions copt;
+ copt.protocol = brpc::PROTOCOL_HTTP;
+ copt.max_retry = 0;
+ brpc::Channel chan;
+ ASSERT_EQ(0, chan.Init(ep, &copt));
+ test::EchoRequest req;
+ test::EchoResponse res;
+ req.set_message(EXP_REQUEST);
+ cntl->http_request().uri() = "/EchoService/Echo";
+ cntl->http_request().set_method(brpc::HTTP_METHOD_POST);
+ cntl->http_request().set_content_type("application/json");
+ chan.CallMethod(nullptr, cntl, &req, &res, nullptr);
+}
+
+// Returns a port nothing is listening on, or -1. `ServerOptions.internal_port`
+// has to be an explicit number, Server::Start() rejects 0 because it stands
+// for an ephemeral port, so ask the system for a free one rather than hardcode
+// a port that another test may be listening on.
+int PickUnusedPort() {
+ butil::fd_guard sockfd(butil::tcp_listen(butil::EndPoint(butil::IP_ANY,
0)));
+ if (sockfd < 0) {
+ return -1;
+ }
+ butil::EndPoint point;
+ if (butil::get_local_side(sockfd, &point) != 0) {
+ return -1;
+ }
+ return point.port;
+}
+
+// Starts `server` on an ephemeral port and fills `options->internal_port` with
+// another one. Both are released before Start() binds them and something else
+// may take one in between, hence the retries. Returns 0 on success.
+int StartWithInternalPort(brpc::Server* server, brpc::ServerOptions* options) {
+ for (int i = 0; i < 10; ++i) {
+ int internal_port = PickUnusedPort();
+ if (internal_port < 0) {
+ continue;
+ }
+ options->internal_port = internal_port;
+ if (0 == server->Start("127.0.0.1:0", options)) {
+ return 0;
+ }
+ }
+ return -1;
+}
Review Comment:
These helpers have external linkage in this TU (unlike the `static` versions
added in `brpc_redis_unittest.cpp`). That can cause link-time symbol collisions
if multiple test TUs are linked into a single binary, and it also unnecessarily
exports test-only symbols. Make these `static` or wrap them in an anonymous
namespace; optionally factor them into a shared test utility header to avoid
duplication across test files.
##########
src/brpc/server.cpp:
##########
@@ -2371,6 +2371,45 @@ bool Server::AcceptRequest(Controller* cntl) const {
return true;
}
+static bool IsBuiltinOrTabbed(const Server::MethodProperty* mp) {
+ return nullptr != mp && (mp->is_builtin_service || mp->params.is_tabbed);
+}
+
+bool Server::RejectBuiltinAccess(Controller* cntl,
+ const MethodProperty* mp) const {
+ if (!cntl->is_security_mode() || !IsBuiltinOrTabbed(mp)) {
+ return false;
+ }
+ cntl->SetFailed(EPERM, "Not allowed to access builtin and Tabbed services,
try "
+ "ServerOptions.internal_port=%d instead if you're
in internal network",
Review Comment:
The message has awkward grammar (“in internal network”) and can be clearer.
Consider changing to “on the internal network” (and optionally rephrasing “try
… instead” to “use …”) to improve readability for end-users encountering this
error.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
