diff --git a/src/brpc/builtin/bad_method_service.cpp b/src/brpc/builtin/bad_method_service.cpp index 5d5ed3260a..dde401f485 100644 --- a/src/brpc/builtin/bad_method_service.cpp +++ b/src/brpc/builtin/bad_method_service.cpp @@ -45,6 +45,15 @@ void BadMethodService::no_method(::google::protobuf::RpcController* cntl_base, std::ostringstream os; os << "Missing method name for service=" << request->service_name() << '.'; + + // Requests from the public port must not learn anything more about the + // services of the server when ServerOptions.internal_port is set, + // especially not the methods of a builtin service. + if (cntl->is_security_mode()) { + cntl->SetFailed(ENOMETHOD, "%s", os.str().c_str()); + return; + } + const Server::ServiceProperty* sp = ServerPrivateAccessor(server) .FindServicePropertyAdaptively(request->service_name()); if (sp != nullptr && sp->service != nullptr) { diff --git a/src/brpc/details/controller_private_accessor.h b/src/brpc/details/controller_private_accessor.h index fc4b9666e2..1aad5b2b4e 100644 --- a/src/brpc/details/controller_private_accessor.h +++ b/src/brpc/details/controller_private_accessor.h @@ -71,31 +71,31 @@ class ControllerPrivateAccessor { return _cntl->_current_call.stream_user_data; } - ControllerPrivateAccessor &set_security_mode(bool security_mode) { + ControllerPrivateAccessor& set_security_mode(bool security_mode) { _cntl->set_flag(Controller::FLAGS_SECURITY_MODE, security_mode); return *this; } - ControllerPrivateAccessor &set_remote_side(const butil::EndPoint& pt) { + ControllerPrivateAccessor& set_remote_side(const butil::EndPoint& pt) { _cntl->_remote_side = pt; return *this; } - ControllerPrivateAccessor &set_local_side(const butil::EndPoint& pt) { + ControllerPrivateAccessor& set_local_side(const butil::EndPoint& pt) { _cntl->_local_side = pt; return *this; } - ControllerPrivateAccessor &set_auth_context(const AuthContext* ctx) { + ControllerPrivateAccessor& set_auth_context(const AuthContext* ctx) { _cntl->set_auth_context(ctx); return *this; } // Overloaded set_span methods to support both shared_ptr and raw pointer - ControllerPrivateAccessor &set_span(const std::shared_ptr& span); - ControllerPrivateAccessor &set_span(Span* span); + ControllerPrivateAccessor& set_span(const std::shared_ptr& span); + ControllerPrivateAccessor& set_span(Span* span); - ControllerPrivateAccessor &set_request_protocol(ProtocolType protocol) { + ControllerPrivateAccessor& set_request_protocol(ProtocolType protocol) { _cntl->_request_protocol = protocol; return *this; } @@ -196,7 +196,7 @@ class ControllerPrivateAccessor { // utility only useable by brpc developers. class RPCSender { public: - virtual ~RPCSender() {} + virtual ~RPCSender() = default; virtual int IssueRPC(int64_t start_realtime_us) = 0; }; diff --git a/src/brpc/details/server_private_accessor.h b/src/brpc/details/server_private_accessor.h index d553b4dcfa..ee7929ddb3 100644 --- a/src/brpc/details/server_private_accessor.h +++ b/src/brpc/details/server_private_accessor.h @@ -104,6 +104,29 @@ class ServerPrivateAccessor { const Server* _server; }; +// 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 stop dispatching the request immediately. +// 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. +inline bool RejectBuiltinAccess(Controller* cntl, const Server& server, + const Server::MethodProperty* mp) { + if (!cntl->is_security_mode() || + (!mp->is_builtin_service && !mp->params.is_tabbed)) { + return false; + } + cntl->SetFailed(EPERM, "Not allowed to access builtin services, try " + "ServerOptions.internal_port=%d instead if you're in " + "internal network", + server.options().internal_port); + return true; +} + // Count one error if release() is not called before destruction of this object. class ScopedNonServiceError { public: diff --git a/src/brpc/nshead_pb_service_adaptor.cpp b/src/brpc/nshead_pb_service_adaptor.cpp index 956e150844..17e29228ab 100644 --- a/src/brpc/nshead_pb_service_adaptor.cpp +++ b/src/brpc/nshead_pb_service_adaptor.cpp @@ -122,6 +122,9 @@ void NsheadPbServiceAdaptor::ProcessNsheadRequest( meta->full_method_name().c_str()); break; } + if (RejectBuiltinAccess(controller, server, sp)) { + break; + } pbdone->status = sp->status; sp->status->OnRequested(); diff --git a/src/brpc/policy/baidu_rpc_protocol.cpp b/src/brpc/policy/baidu_rpc_protocol.cpp index f39463e888..5a6451b0d5 100644 --- a/src/brpc/policy/baidu_rpc_protocol.cpp +++ b/src/brpc/policy/baidu_rpc_protocol.cpp @@ -714,13 +714,13 @@ void ProcessRpcRequest(InputMessageBase* msg_base) { google::protobuf::Service* svc = nullptr; google::protobuf::MethodDescriptor* method = nullptr; if (nullptr != server->options().baidu_master_service) { - if (socket->is_overcrowded() && + if (socket->is_overcrowded() && !server->options().ignore_eovercrowded && !server->options().baidu_master_service->ignore_eovercrowded()) { - cntl->SetFailed(EOVERCROWDED, "Connection to %s is overcrowded", - butil::endpoint2str(socket->remote_side()).c_str()); + cntl->SetFailed(EOVERCROWDED, "Connection to %s is overcrowded", + butil::endpoint2str(socket->remote_side()).c_str()); break; - } + } svc = server->options().baidu_master_service; auto sampled_request = new SampledRequest; sampled_request->meta.set_service_name(request_meta.service_name()); @@ -732,10 +732,8 @@ void ProcessRpcRequest(InputMessageBase* msg_base) { if (method_status) { int rejected_cc = 0; if (!method_status->OnRequested(&rejected_cc, cntl.get())) { - cntl->SetFailed( - ELIMIT, - "Rejected by %s's ConcurrencyLimiter, concurrency=%d", - butil::class_name(), rejected_cc); + cntl->SetFailed(ELIMIT, "Rejected by %s's ConcurrencyLimiter, concurrency=%d", + butil::class_name(), rejected_cc); break; } } @@ -744,9 +742,8 @@ void ProcessRpcRequest(InputMessageBase* msg_base) { } messages = BaiduProxyPBMessages::Get(); - msg->payload.cutn( - &((SerializedRequest*)messages->Request())->serialized_data(), - req_size - meta.attachment_size()); + msg->payload.cutn(&((SerializedRequest*)messages->Request())->serialized_data(), + req_size - meta.attachment_size()); if (!msg->payload.empty()) { cntl->request_attachment().swap(msg->payload); } @@ -759,7 +756,7 @@ void ProcessRpcRequest(InputMessageBase* msg_base) { server_accessor.FindServicePropertyByName(svc_name); if (nullptr == sp) { cntl->SetFailed(ENOSERVICE, "Fail to find service=%s", - request_meta.service_name().c_str()); + request_meta.service_name().c_str()); break; } svc_name = sp->service->GetDescriptor()->full_name(); @@ -772,7 +769,11 @@ void ProcessRpcRequest(InputMessageBase* msg_base) { request_meta.service_name().c_str(), request_meta.method_name().c_str()); break; - } else if (mp->service->GetDescriptor() == BadMethodService::descriptor()) { + } + if (RejectBuiltinAccess(cntl.get(), *server, mp)) { + break; + } + if (mp->service->GetDescriptor() == BadMethodService::descriptor()) { BadMethodRequest breq; BadMethodResponse bres; breq.set_service_name(request_meta.service_name()); diff --git a/src/brpc/policy/http_rpc_protocol.cpp b/src/brpc/policy/http_rpc_protocol.cpp index 9d63de23e9..f09f2c8386 100644 --- a/src/brpc/policy/http_rpc_protocol.cpp +++ b/src/brpc/policy/http_rpc_protocol.cpp @@ -1573,6 +1573,13 @@ void ProcessHttpRequest(InputMessageBase *msg) { } return; } else if (mp->service->GetDescriptor() == BadMethodService::descriptor()) { + // NOTE: Unlike pb protocols, a http request falls back to + // BadMethodService whenever the URL only carries a service name, + // no matter whether the service is builtin or not. Rejecting it here + // would turn a helpful "missing method name" hint into a confusing + // "not allowed to access builtin services" for normal services, so the + // request is dispatched instead and BadMethodService itself hides the + // list of available methods in security mode. BadMethodRequest breq; BadMethodResponse bres; butil::StringSplitter split(path.c_str(), '/'); @@ -1580,6 +1587,9 @@ void ProcessHttpRequest(InputMessageBase *msg) { mp->service->CallMethod(mp->method, cntl, &breq, &bres, nullptr); return; } + if (RejectBuiltinAccess(cntl, *server, mp)) { + return; + } // Switch to service-specific error. non_service_error.release(); MethodStatus* method_status = mp->status; @@ -1620,11 +1630,6 @@ void ProcessHttpRequest(InputMessageBase *msg) { if (!server->AcceptRequest(cntl)) { return; } - } else if (security_mode) { - cntl->SetFailed(EPERM, "Not allowed to access builtin services, try " - "ServerOptions.internal_port=%d instead if you're in" - " internal network", server->options().internal_port); - return; } google::protobuf::Service* svc = mp->service; diff --git a/src/brpc/policy/hulu_pbrpc_protocol.cpp b/src/brpc/policy/hulu_pbrpc_protocol.cpp index cb397a49b2..4bacd9e521 100644 --- a/src/brpc/policy/hulu_pbrpc_protocol.cpp +++ b/src/brpc/policy/hulu_pbrpc_protocol.cpp @@ -449,8 +449,11 @@ void ProcessHuluRequest(InputMessageBase* msg_base) { cntl->SetFailed(ENOMETHOD, "Fail to find method=%d of service=%s", meta.method_index(), meta.service_name().c_str()); break; - } else if (sp->service->GetDescriptor() - == BadMethodService::descriptor()) { + } + if (RejectBuiltinAccess(cntl.get(), *server, sp)) { + break; + } + if (sp->service->GetDescriptor() == BadMethodService::descriptor()) { BadMethodRequest breq; BadMethodResponse bres; breq.set_service_name(meta.service_name()); diff --git a/src/brpc/policy/mongo_protocol.cpp b/src/brpc/policy/mongo_protocol.cpp index cae64b5b06..3c7262d401 100644 --- a/src/brpc/policy/mongo_protocol.cpp +++ b/src/brpc/policy/mongo_protocol.cpp @@ -197,11 +197,11 @@ void ProcessMongoRequest(InputMessageBase* msg_base) { << " of MongoService should be equal to 1!"; } - const Server::MethodProperty *mp = - ServerPrivateAccessor(server) - .FindMethodPropertyByFullName(srv_des->method(0)->full_name()); + ServerPrivateAccessor server_accessor(server); + const Server::MethodProperty *mp = server_accessor.FindMethodPropertyByFullName( + srv_des->method(0)->full_name()); - MongoContextMessage *context_msg = + MongoContextMessage* context_msg = dynamic_cast(socket->parsing_context()); if (nullptr == context_msg) { LOG(WARNING) << "socket context wasn't set correctly"; @@ -212,8 +212,10 @@ void ProcessMongoRequest(InputMessageBase* msg_base) { mongo_done->cntl.set_mongo_session_data(context_msg->context()); ControllerPrivateAccessor accessor(&(mongo_done->cntl)); + const bool security_mode = server->options().security_mode() && + socket->user() == server_accessor.acceptor(); accessor.set_server(server) - .set_security_mode(server->options().security_mode()) + .set_security_mode(security_mode) .set_peer_id(socket->id()) .set_remote_side(socket->remote_side()) .set_local_side(socket->local_side()) @@ -233,7 +235,7 @@ void ProcessMongoRequest(InputMessageBase* msg_base) { break; } - if (!ServerPrivateAccessor(server).AddConcurrency(&(mongo_done->cntl))) { + if (!server_accessor.AddConcurrency(&(mongo_done->cntl))) { mongo_done->cntl.SetFailed( ELIMIT, "Reached server's max_concurrency=%d", server->options().max_concurrency); diff --git a/src/brpc/policy/sofa_pbrpc_protocol.cpp b/src/brpc/policy/sofa_pbrpc_protocol.cpp index 6b663c19f9..d0c42cc4f8 100644 --- a/src/brpc/policy/sofa_pbrpc_protocol.cpp +++ b/src/brpc/policy/sofa_pbrpc_protocol.cpp @@ -410,6 +410,9 @@ void ProcessSofaRequest(InputMessageBase* msg_base) { meta.method().c_str()); break; } + if (RejectBuiltinAccess(cntl.get(), *server, sp)) { + break; + } if (socket->is_overcrowded() && !server->options().ignore_eovercrowded && !sp->ignore_eovercrowded) { diff --git a/test/brpc_server_unittest.cpp b/test/brpc_server_unittest.cpp index ed0268e853..8e386b2a8c 100644 --- a/test/brpc_server_unittest.cpp +++ b/test/brpc_server_unittest.cpp @@ -1483,6 +1483,228 @@ TEST_F(ServerTest, add_builtin_service) { } } +// Call the builtin `brpc.version` service through a pb protocol. +void CallVersionByPb(const butil::EndPoint& ep, + brpc::ProtocolType protocol, + brpc::Controller* cntl) { + brpc::ChannelOptions copt; + copt.protocol = protocol; + copt.max_retry = 0; + brpc::Channel chan; + ASSERT_EQ(0, chan.Init(ep, &copt)); + brpc::VersionRequest req; + brpc::VersionResponse res; + brpc::version_Stub stub(&chan); + stub.default_method(cntl, &req, &res, nullptr); +} + +// Call the same builtin service the way a browser would. +void CallVersionByHttp(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)); + cntl->http_request().uri() = "/version"; + chan.CallMethod(nullptr, cntl, nullptr, nullptr, nullptr); +} + +// Call an ordinary (non-builtin) service through a pb protocol. +void CallEchoByPb(const butil::EndPoint& ep, + brpc::ProtocolType protocol, + brpc::Controller* cntl) { + brpc::ChannelOptions copt; + copt.protocol = protocol; + 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); + test::EchoService_Stub stub(&chan); + stub.Echo(cntl, &req, &res, nullptr); +} + +// Builtin services must be gated by ServerOptions.internal_port no matter +// which protocol carries the request. +TEST_F(ServerTest, builtin_services_are_gated_by_internal_port) { + const struct { + brpc::ProtocolType protocol; + const char* name; + } cases[] = { + { brpc::PROTOCOL_BAIDU_STD, "baidu_std" }, + { brpc::PROTOCOL_HULU_PBRPC, "hulu_pbrpc" }, + { brpc::PROTOCOL_SOFA_PBRPC, "sofa_pbrpc" }, + }; + + butil::EndPoint ep; + 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)); + ASSERT_TRUE(server.options().security_mode()); + + for (size_t i = 0; i < arraysize(cases); ++i) { + // Not reachable from the public port ... + brpc::Controller cntl; + CallVersionByPb(ep, cases[i].protocol, &cntl); + ASSERT_EQ(EPERM, cntl.ErrorCode()) + << cases[i].name << ": " << cntl.ErrorText(); + + // ... but reachable from internal_port. + cntl.Reset(); + CallVersionByPb(internal_ep, cases[i].protocol, &cntl); + ASSERT_FALSE(cntl.Failed()) + << cases[i].name << ": " << cntl.ErrorText(); + + // Ordinary services on the public port are unaffected. + cntl.Reset(); + CallEchoByPb(ep, cases[i].protocol, &cntl); + ASSERT_FALSE(cntl.Failed()) + << cases[i].name << ": " << cntl.ErrorText(); + } + + // http was gated before, make sure it stays that way. + brpc::Controller cntl; + CallVersionByHttp(ep, &cntl); + ASSERT_TRUE(cntl.Failed()); + ASSERT_EQ(brpc::HTTP_STATUS_FORBIDDEN, cntl.http_response().status_code()) + << cntl.ErrorText(); + cntl.Reset(); + CallVersionByHttp(internal_ep, &cntl); + ASSERT_FALSE(cntl.Failed()) << cntl.ErrorText(); + + ASSERT_EQ(0, server.Stop(0)); + ASSERT_EQ(0, server.Join()); +} + +// A service-name-only URL is dispatched to the builtin BadMethodService which +// lists the methods of the service. +void CallServiceWithoutMethodByHttp(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)); + cntl->http_request().uri() = "/EchoService"; + chan.CallMethod(nullptr, cntl, nullptr, nullptr, nullptr); +} + +// Ask for the builtin BadMethodService on purpose, which a pb client can do +// since the service is registered like any other builtin service. +void CallBadMethodByPb(const butil::EndPoint& ep, + brpc::ProtocolType protocol, + brpc::Controller* cntl) { + brpc::ChannelOptions copt; + copt.protocol = protocol; + copt.max_retry = 0; + brpc::Channel chan; + ASSERT_EQ(0, chan.Init(ep, &copt)); + brpc::BadMethodRequest req; + brpc::BadMethodResponse res; + req.set_service_name("EchoService"); + brpc::badmethod_Stub stub(&chan); + stub.no_method(cntl, &req, &res, nullptr); +} + +// BadMethodService is builtin as well and it lists the methods of a service, +// so it must not be reachable from the public port in security mode. The http +// fallback to BadMethodService is an exception: it must keep reporting the +// missing method name, only without listing the methods. +TEST_F(ServerTest, bad_method_does_not_leak_methods_in_security_mode) { + butil::EndPoint ep; + 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)); + ASSERT_TRUE(server.options().security_mode()); + + const struct { + brpc::ProtocolType protocol; + const char* name; + } cases[] = { + { brpc::PROTOCOL_BAIDU_STD, "baidu_std" }, + { brpc::PROTOCOL_HULU_PBRPC, "hulu_pbrpc" }, + { brpc::PROTOCOL_SOFA_PBRPC, "sofa_pbrpc" }, + }; + for (size_t i = 0; i < arraysize(cases); ++i) { + brpc::Controller cntl; + CallBadMethodByPb(ep, cases[i].protocol, &cntl); + ASSERT_EQ(EPERM, cntl.ErrorCode()) + << cases[i].name << ": " << cntl.ErrorText(); + ASSERT_EQ(std::string::npos, cntl.ErrorText().find("Available methods")) + << cases[i].name << ": " << cntl.ErrorText(); + + cntl.Reset(); + CallBadMethodByPb(internal_ep, cases[i].protocol, &cntl); + ASSERT_EQ(brpc::ENOMETHOD, cntl.ErrorCode()) + << cases[i].name << ": " << cntl.ErrorText(); + ASSERT_NE(std::string::npos, cntl.ErrorText().find("Available methods")) + << cases[i].name << ": " << cntl.ErrorText(); + } + + // http still tells that the method name is missing, but without listing + // the methods of the service. + brpc::Controller cntl; + CallServiceWithoutMethodByHttp(ep, &cntl); + ASSERT_TRUE(cntl.Failed()); + ASSERT_EQ(brpc::HTTP_STATUS_NOT_FOUND, cntl.http_response().status_code()) + << cntl.ErrorText(); + ASSERT_NE(std::string::npos, cntl.ErrorText().find("Missing method name")) + << cntl.ErrorText(); + ASSERT_EQ(std::string::npos, cntl.ErrorText().find("Available methods")) + << cntl.ErrorText(); + + cntl.Reset(); + CallServiceWithoutMethodByHttp(internal_ep, &cntl); + ASSERT_TRUE(cntl.Failed()); + ASSERT_EQ(brpc::HTTP_STATUS_NOT_FOUND, cntl.http_response().status_code()) + << cntl.ErrorText(); + ASSERT_NE(std::string::npos, cntl.ErrorText().find("Available methods")) + << cntl.ErrorText(); + + ASSERT_EQ(0, server.Stop(0)); + ASSERT_EQ(0, server.Join()); +} + +// Without internal_port the server is not in security mode and builtin +// services stay reachable from the only port, which is the default behavior. +TEST_F(ServerTest, builtin_services_are_open_without_internal_port) { + butil::EndPoint ep; + ASSERT_EQ(0, str2endpoint("127.0.0.1:8613", &ep)); + + brpc::Server server; + EchoServiceImpl echo_svc; + ASSERT_EQ(0, server.AddService(&echo_svc, brpc::SERVER_DOESNT_OWN_SERVICE)); + ASSERT_EQ(0, server.Start(ep, nullptr)); + ASSERT_FALSE(server.options().security_mode()); + + brpc::Controller cntl; + CallVersionByPb(ep, brpc::PROTOCOL_BAIDU_STD, &cntl); + ASSERT_FALSE(cntl.Failed()) << cntl.ErrorText(); + + cntl.Reset(); + CallVersionByHttp(ep, &cntl); + ASSERT_FALSE(cntl.Failed()) << cntl.ErrorText(); + + ASSERT_EQ(0, server.Stop(0)); + ASSERT_EQ(0, server.Join()); +} + TEST_F(ServerTest, base64_to_string) { // We test two cases as following. If these two tests can be passed, we // can prove that the pb_bytes_to_base64 flag is working in both client side