Skip to content

Enforce internal_port gating of builtin services in pb protocols #3511

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
chenBright wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Enforce internal_port gating of builtin services in pb protocols #3511

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/brpc/builtin/bad_method_service.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
16 changes: 8 additions & 8 deletions src/brpc/details/controller_private_accessor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>& span);
ControllerPrivateAccessor &set_span(Span* span);
ControllerPrivateAccessor& set_span(const std::shared_ptr<Span>& span);
ControllerPrivateAccessor& set_span(Span* span);

ControllerPrivateAccessor &set_request_protocol(ProtocolType protocol) {
ControllerPrivateAccessor& set_request_protocol(ProtocolType protocol) {
_cntl->_request_protocol = protocol;
return *this;
}
Expand Down Expand Up @@ -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;
};

Expand Down
23 changes: 23 additions & 0 deletions src/brpc/details/server_private_accessor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
3 changes: 3 additions & 0 deletions src/brpc/nshead_pb_service_adaptor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,9 @@ void NsheadPbServiceAdaptor::ProcessNsheadRequest(
meta->full_method_name().c_str());
break;
}
if (RejectBuiltinAccess(controller, server, sp)) {
break;
}
Comment thread
chenBright marked this conversation as resolved.
pbdone->status = sp->status;
sp->status->OnRequested();

Expand Down
27 changes: 14 additions & 13 deletions src/brpc/policy/baidu_rpc_protocol.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand All @@ -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<BaiduMasterService>(), rejected_cc);
cntl->SetFailed(ELIMIT, "Rejected by %s's ConcurrencyLimiter, concurrency=%d",
butil::class_name<BaiduMasterService>(), rejected_cc);
break;
}
}
Expand All @@ -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);
}
Expand All @@ -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();
Expand All @@ -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());
Expand Down
15 changes: 10 additions & 5 deletions src/brpc/policy/http_rpc_protocol.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1573,13 +1573,23 @@ 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(), '/');
breq.set_service_name(std::string(split.field(), split.length()));
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;
Expand Down Expand Up @@ -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;
}
Comment thread
chenBright marked this conversation as resolved.

google::protobuf::Service* svc = mp->service;
Expand Down
7 changes: 5 additions & 2 deletions src/brpc/policy/hulu_pbrpc_protocol.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
14 changes: 8 additions & 6 deletions src/brpc/policy/mongo_protocol.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<MongoContextMessage*>(socket->parsing_context());
if (nullptr == context_msg) {
LOG(WARNING) << "socket context wasn't set correctly";
Expand All @@ -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())
Expand All @@ -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);
Expand Down
3 changes: 3 additions & 0 deletions src/brpc/policy/sofa_pbrpc_protocol.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Loading
Loading