Copilot commented on code in PR #3525:
URL: https://github.com/apache/brpc/pull/3525#discussion_r3943610277
##########
src/brpc/server.h:
##########
@@ -616,6 +622,35 @@ 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 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.
+ bool RejectBuiltinAccess(Controller* cntl, const MethodProperty* mp) const;
+
+ // 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.
Review Comment:
This helper only enforces the internal_port restriction where protocols
actually call it. Several server-side protocols still dispatch non-builtin
services without invoking RejectNonBuiltinAccessFromInternalPort (e.g.,
src/brpc/policy/mongo_protocol.cpp:183+ and
src/brpc/policy/redis_protocol.cpp:117+), which means internal_port can still
serve ordinary services for those protocols despite the updated contract/docs.
##########
src/brpc/server.h:
##########
@@ -616,6 +622,35 @@ 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 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.
+ bool RejectBuiltinAccess(Controller* cntl, const MethodProperty* mp) const;
+
+ // 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.
+ // Returns true if the access was rejected, in which case `cntl` was
already etFailed()
+ // and the caller must stop dispatching the request immediately.
Review Comment:
Typo in the comment: it says `etFailed()` but the API is `SetFailed()`. This
is in a public header comment and should be corrected to avoid confusion.
##########
src/brpc/server.cpp:
##########
@@ -2371,6 +2371,38 @@ bool Server::AcceptRequest(Controller* cntl) const {
return true;
}
+bool Server::RejectBuiltinAccess(Controller* cntl,
+ const MethodProperty* mp) const {
+ 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",
+ _options.internal_port);
+ return true;
+}
+
+bool Server::RejectNonBuiltinAccessFromInternalPort(
+ Controller* cntl, const MethodProperty* mp) const {
+ if (mp->is_builtin_service || mp->params.is_tabbed) {
+ return false;
+ }
+ return RejectNonBuiltinAccessFromInternalPort(cntl);
+}
+
+bool Server::RejectNonBuiltinAccessFromInternalPort(Controller* cntl) const {
+ if (_options.internal_port < 0 ||
+ cntl->local_side().port != _options.internal_port) {
+ return false;
+ }
+ 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);
Review Comment:
The rejection message says "Only builtin services" but the code explicitly
allows Tabbed services on internal_port (see
RejectNonBuiltinAccessFromInternalPort(Controller*, MethodProperty*) returning
false when mp->params.is_tabbed). The message should match the
documented/actual allowed set to avoid misleading operators.
--
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]
