Serve builtin services only on ServerOptions.internal_port

docs/cn/server.md

@@ -686,9 +686,17 @@ pthread模式可以让一些老代码快速尝试brpc，但我们仍然建议逐
 - 设置内部端口。把ServerOptions.internal_port设为一个**仅允许内网访问**的端口。你可通过internal_port访问到内置服务，但通过对外端口(Server.Start时传入的那个)访问内置服务时将看到如下错误：
 
   ```
-  [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
   ```
 
+  反过来，internal_port只提供内置服务(以及Tabbed服务)，普通服务的请求打到这个端口上会被拒绝：
+
+  ```
+  Only builtin services are accessible on ServerOptions.internal_port=..., send the request to the port passed to Server::Start() instead
+  ```
+
+  这是必须的：internal_port上的内置服务请求不需要通过ServerOptions.auth的鉴权，而鉴权结果是记在连接上的，一条连接只在第一个请求时鉴权一次。如果普通服务也在这个端口上提供，那么先发一个内置服务请求就能把整条连接标记为已鉴权，后续在同一条连接上访问普通服务将完全跳过鉴权。
+
 - http proxy指定转发路径。nginx等可配置URL的映射关系，比如下面的配置把访问/MyAPI的外部流量映射到`target-server`的`/ServiceName/MethodName`。当外部流量尝试访问内置服务，比如说/status时，将直接被nginx拒绝。
 ```nginx
   location /MyAPI {

docs/en/server.md

@@ -680,9 +680,17 @@ Builtin services are useful, on the other hand include a lot of internal informa
 - Set internal port. Set ServerOptions.internal_port to a port which can **only be accessible from internal**. You can view builtin services via internal_port, while accesses from the public port (the one passed to Server.Start) should see following error:
 
   ```
-  [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
   ```
 
+  Conversely internal_port serves builtin (and Tabbed) services only, requests for ordinary services sent to it are rejected with:
+
+  ```
+  Only builtin services are accessible on ServerOptions.internal_port=..., send the request to the port passed to Server::Start() instead
+  ```
+
+  This is necessary: builtin requests on internal_port skip the authentication of ServerOptions.auth, and the verdict is remembered per connection since a connection is only authenticated once, on its first request. Were ordinary services served there as well, sending a builtin request first would mark the whole connection as authenticated and every later request on it would bypass authentication entirely.
+
 - http proxies only proxy specified URLs. nginx etc is able to configure how to map different URLs to back-end servers. For example the configure below maps public traffic to /MyAPI to `/ServiceName/MethodName` of `target-server`. If builtin services like /status are accessed from public, nginx rejects the attempts directly.
 ```nginx
   location /MyAPI {

src/brpc/details/server_private_accessor.h

@@ -104,29 +104,6 @@ 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:

src/brpc/nshead_pb_service_adaptor.cpp

@@ -114,22 +114,23 @@ void NsheadPbServiceAdaptor::ProcessNsheadRequest(
         }
 
         ServerPrivateAccessor server_accessor(&server);
-        const Server::MethodProperty *sp = server_accessor
+        const Server::MethodProperty* mp = server_accessor
             .FindMethodPropertyByFullName(meta->full_method_name());
-        if (nullptr == sp ||
-            sp->service->GetDescriptor() == BadMethodService::descriptor()) {
+        if (nullptr == mp ||
+            mp->service->GetDescriptor() == BadMethodService::descriptor()) {
             controller->SetFailed(ENOMETHOD, "Fail to find method=%s", 
                                   meta->full_method_name().c_str());
             break;
         }
-        if (RejectBuiltinAccess(controller, server, sp)) {
+        if (server.RejectBuiltinAccess(controller, mp) ||
+            server.RejectNonBuiltinAccessFromInternalPort(controller, mp)) {
             break;
         }
-        pbdone->status = sp->status;
-        sp->status->OnRequested();
+        pbdone->status = mp->status;
+        mp->status->OnRequested();
 
-        google::protobuf::Service* svc = sp->service;
-        const google::protobuf::MethodDescriptor* method = sp->method;
+        google::protobuf::Service* svc = mp->service;
+        const google::protobuf::MethodDescriptor* method = mp->method;
         ControllerPrivateAccessor(controller).set_method(method);
         done->SetMethodName(butil::EnsureString(method->full_name()));
         pbdone->pbreq.reset(svc->GetRequestPrototype(method).New());

src/brpc/policy/baidu_rpc_protocol.cpp

@@ -770,7 +770,8 @@ void ProcessRpcRequest(InputMessageBase* msg_base) {
                                 request_meta.method_name().c_str());
                 break;
             }
-            if (RejectBuiltinAccess(cntl.get(), *server, mp)) {
+            if (server->RejectBuiltinAccess(cntl.get(), mp) ||
+                server->RejectNonBuiltinAccessFromInternalPort(cntl.get(), mp)) {
                 break;
             }
             if (mp->service->GetDescriptor() == BadMethodService::descriptor()) {

src/brpc/policy/http_rpc_protocol.cpp

@@ -1605,7 +1605,8 @@ void ProcessHttpRequest(InputMessageBase *msg) {
         mp->service->CallMethod(mp->method, cntl, &breq, &bres, nullptr);
         return;
     }
-    if (RejectBuiltinAccess(cntl, *server, mp)) {
+    if (server->RejectBuiltinAccess(cntl, mp) ||
+        server->RejectNonBuiltinAccessFromInternalPort(cntl, mp)) {
         return;
     }
     // Switch to service-specific error.

src/brpc/policy/hulu_pbrpc_protocol.cpp

@@ -442,36 +442,37 @@ void ProcessHuluRequest(InputMessageBase* msg_base) {
             break;
         }
 
-        const Server::MethodProperty *sp =
+        const Server::MethodProperty* mp =
             server_accessor.FindMethodPropertyByNameAndIndex(
                 meta.service_name(), meta.method_index());
-        if (nullptr == sp) {
+        if (nullptr == mp) {
             cntl->SetFailed(ENOMETHOD, "Fail to find method=%d of service=%s",
                             meta.method_index(), meta.service_name().c_str());
             break;
         }
-        if (RejectBuiltinAccess(cntl.get(), *server, sp)) {
+        if (server->RejectBuiltinAccess(cntl.get(), mp) ||
+            server->RejectNonBuiltinAccessFromInternalPort(cntl.get(), mp)) {
             break;
         }
-        if (sp->service->GetDescriptor() == BadMethodService::descriptor()) {
+        if (mp->service->GetDescriptor() == BadMethodService::descriptor()) {
             BadMethodRequest breq;
             BadMethodResponse bres;
             breq.set_service_name(meta.service_name());
-            sp->service->CallMethod(sp->method, cntl.get(), &breq, &bres, nullptr);
+            mp->service->CallMethod(mp->method, cntl.get(), &breq, &bres, nullptr);
             break;
         }
         if (socket->is_overcrowded() &&
             !server->options().ignore_eovercrowded &&
-            !sp->ignore_eovercrowded) {
+            !mp->ignore_eovercrowded) {
             cntl->SetFailed(EOVERCROWDED, "Connection to %s is overcrowded",
                             butil::endpoint2str(socket->remote_side()).c_str());
             break;
         }
 
         // Switch to service-specific error.
         non_service_error.release();
-        method_status = sp->status;
-        const google::protobuf::MethodDescriptor* method = sp->method;
+        method_status = mp->status;
+        const google::protobuf::MethodDescriptor* method = mp->method;
         const std::string method_full_name = butil::EnsureString(method->full_name());
         if (method_status) {
             int rejected_cc = 0;
@@ -482,7 +483,7 @@ void ProcessHuluRequest(InputMessageBase* msg_base) {
             }
         }
 
-        google::protobuf::Service* svc = sp->service;
+        google::protobuf::Service* svc = mp->service;
         accessor.set_method(method);
 
         if (!server->AcceptRequest(cntl.get())) {

src/brpc/policy/nshead_protocol.cpp

@@ -314,6 +314,9 @@ void ProcessNsheadRequest(InputMessageBase* msg_base) {
             cntl->SetFailed(ELOGOFF, "Server is stopping");
             break;
         }
+        if (server->RejectNonBuiltinAccessFromInternalPort(cntl)) {
+            break;
+        }
         if (socket->is_overcrowded() && !server->options().ignore_eovercrowded) {
             cntl->SetFailed(EOVERCROWDED, "Connection to %s is overcrowded",
                             butil::endpoint2str(socket->remote_side()).c_str());

src/brpc/policy/sofa_pbrpc_protocol.cpp

@@ -403,36 +403,37 @@ void ProcessSofaRequest(InputMessageBase* msg_base) {
             break;
         }
 
-        const Server::MethodProperty *sp =
+        const Server::MethodProperty* mp =
             server_accessor.FindMethodPropertyByFullName(meta.method());
-        if (nullptr == sp) {
+        if (nullptr == mp) {
             cntl->SetFailed(ENOMETHOD, "Fail to find method=%s", 
                             meta.method().c_str());
             break;
         }
-        if (RejectBuiltinAccess(cntl.get(), *server, sp)) {
+        if (server->RejectBuiltinAccess(cntl.get(), mp) ||
+            server->RejectNonBuiltinAccessFromInternalPort(cntl.get(), mp)) {
             break;
         }
         if (socket->is_overcrowded() &&
             !server->options().ignore_eovercrowded &&
-            !sp->ignore_eovercrowded) {
+            !mp->ignore_eovercrowded) {
             cntl->SetFailed(EOVERCROWDED, "Connection to %s is overcrowded",
                             butil::endpoint2str(socket->remote_side()).c_str());
             break;
         }
         // Switch to service-specific error.
         non_service_error.release();
-        method_status = sp->status;
+        method_status = mp->status;
         if (method_status) {
             int rejected_cc = 0;
             if (!method_status->OnRequested(&rejected_cc)) {
                 cntl->SetFailed(ELIMIT, "Rejected by %s's ConcurrencyLimiter, concurrency=%d",
-                                butil::EnsureString(sp->method->full_name()).c_str(), rejected_cc);
+                                butil::EnsureString(mp->method->full_name()).c_str(), rejected_cc);
                 break;
             }
         }
-        google::protobuf::Service* svc = sp->service;
-        const google::protobuf::MethodDescriptor* method = sp->method;
+        google::protobuf::Service* svc = mp->service;
+        const google::protobuf::MethodDescriptor* method = mp->method;
         accessor.set_method(method);
 
         if (!server->AcceptRequest(cntl.get())) {

src/brpc/policy/thrift_protocol.cpp

@@ -510,6 +510,9 @@ void ProcessThriftRequest(InputMessageBase* msg_base) {
             " ServerOptions.thrift_service, close the connection.";
         return cntl->SetFailed(EINTERNAL, "ServerOptions.thrift_service is NULL");
     }
+    if (server->RejectNonBuiltinAccessFromInternalPort(cntl)) {
+        return;
+    }
 
     // Switch to service-specific error.
     non_service_error.release();

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);
+    return true;
+}
+
 #ifdef SSL_CTRL_SET_TLSEXT_HOSTNAME
 int Server::SSLSwitchCTXByHostname(struct ssl_st* ssl,
                                    int* al, void* se) {

src/brpc/server.h

@@ -202,6 +202,12 @@ struct ServerOptions {
     // hiding them from public. Setting this option also enables security
     // protection code which we may add constantly.
     // Update: this option affects Tabbed services as well.
+    // 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.
     // Default: -1
     int internal_port;
 
@@ -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.
+    // NOTE: Same 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;
+
     bool has_progressive_read_method() const {
         return this->_has_progressive_read_method;
     }

test/brpc_http_rpc_protocol_unittest.cpp

@@ -563,6 +563,35 @@ TEST_F(HttpTest, builtin_auth_policy_on_public_and_internal_port) {
         ASSERT_TRUE(protected_cntl.Failed());
     }
 
+    {
+        // A builtin request is exempted from authentication on internal_port
+        // and its verdict latches the whole connection, so the exemption would
+        // carry over to whatever is sent next on that very connection. Only
+        // builtin services are served there, which keeps the latch harmless.
+        const std::string connection_group = "builtin-auth-policy-internal";
+        brpc::Channel builtin_channel;
+        brpc::Channel protected_channel;
+        brpc::ChannelOptions copt;
+        copt.protocol = brpc::PROTOCOL_HTTP;
+        copt.connection_type = brpc::CONNECTION_TYPE_POOLED;
+        copt.connection_group = connection_group;
+        copt.max_retry = 0;
+        ASSERT_EQ(0, builtin_channel.Init(internal_ep, &copt));
+        ASSERT_EQ(0, protected_channel.Init(internal_ep, &copt));
+
+        brpc::Controller builtin_cntl;
+        CallVersion(&builtin_channel, &builtin_cntl);
+        ASSERT_FALSE(builtin_cntl.Failed()) << builtin_cntl.ErrorText();
+        ASSERT_EQ(brpc::HTTP_STATUS_OK, builtin_cntl.http_response().status_code());
+
+        brpc::Controller protected_cntl;
+        CallHttpEcho(&protected_channel, &protected_cntl);
+        ASSERT_TRUE(protected_cntl.Failed());
+        ASSERT_EQ(brpc::EHTTP, protected_cntl.ErrorCode()) << protected_cntl.ErrorText();
+        ASSERT_EQ(brpc::HTTP_STATUS_FORBIDDEN,
+                  protected_cntl.http_response().status_code());
+    }
+
     ASSERT_EQ(0, server.Stop(0));
     ASSERT_EQ(0, server.Join());
     brpc::FLAGS_max_connection_pool_size = saved_max_connection_pool_size;