Skip to content

Commit a47e349

Browse files
wwbmmmwwbmmm
authored
fix(http): apply auth check to builtin requests on public port (#3308)
* fix(http): apply auth check to builtin requests on public port Only skip auth for builtin HTTP requests received from internal_port. Builtin requests coming from the server listening port now follow the same auth flow as other public HTTP requests. Also add regression coverage for builtin HTTP auth behavior on both the public listener and internal_port. * Fix http_rpc_protocol unittest failed * Fix builtin_auth_policy_on_public_and_internal_port test case not stable * Fix builtin_auth_policy_on_public_and_internal_port asan run fail
1 parent 3aa5dab commit a47e349

2 files changed

Lines changed: 156 additions & 4 deletions

File tree

‎src/brpc/policy/http_rpc_protocol.cpp‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1387,10 +1387,12 @@ bool VerifyHttpRequest(const InputMessageBase* msg) {
13871387
http_request->header().uri().path(), server, NULL);
13881388
if (mp != NULL && mp->is_builtin_service &&
13891389
mp->service->GetDescriptor() != BadMethodService::descriptor()) {
1390-
// BuiltinService doesn't need authentication
1391-
// TODO: Fix backdoor that sends BuiltinService at first
1392-
// and then sends other requests without authentication
1393-
return true;
1390+
// Builtin services on internal_port doesn't need authentication
1391+
// Builtin services on the public listener must pass authentication
1392+
if (server->options().internal_port >= 0 &&
1393+
socket->local_side().port == server->options().internal_port) {
1394+
return true;
1395+
}
13941396
}
13951397

13961398
const std::string *authorization

‎test/brpc_http_rpc_protocol_unittest.cpp‎

Lines changed: 150 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ DECLARE_bool(rpc_dump);
6060
DECLARE_string(rpc_dump_dir);
6161
DECLARE_int32(rpc_dump_max_requests_in_one_file);
6262
DECLARE_bool(allow_chunked_length);
63+
DECLARE_int32(max_connection_pool_size);
6364
extern bvar::CollectorSpeedLimit g_rpc_dump_sl;
6465
}
6566

@@ -163,6 +164,23 @@ class HttpTest : public ::testing::Test{
163164
EXPECT_EQ(expect, brpc::policy::VerifyHttpRequest(msg));
164165
}
165166

167+
void VerifyMessageFromLocalPort(brpc::InputMessageBase* msg,
168+
bool expect,
169+
int local_port) {
170+
brpc::SocketId id;
171+
brpc::SocketOptions options;
172+
options.fd = dup(_pipe_fds[1]);
173+
EXPECT_GE(options.fd, 0);
174+
options.local_side = butil::EndPoint(butil::my_ip(), local_port);
175+
EXPECT_EQ(0, brpc::Socket::Create(options, &id));
176+
177+
brpc::SocketUniquePtr socket;
178+
EXPECT_EQ(0, brpc::Socket::Address(id, &socket));
179+
socket->ReAddress(&msg->_socket);
180+
msg->_arg = &_server;
181+
EXPECT_EQ(expect, brpc::policy::VerifyHttpRequest(msg));
182+
}
183+
166184
void ProcessMessage(void (*process)(brpc::InputMessageBase*),
167185
brpc::InputMessageBase* msg, bool set_eof) {
168186
if (msg->_socket == NULL) {
@@ -225,6 +243,33 @@ class HttpTest : public ::testing::Test{
225243
return msg;
226244
}
227245

246+
void InitHttpPooledChannel(brpc::Channel* channel,
247+
const butil::EndPoint& ep,
248+
const std::string& connection_group) {
249+
brpc::ChannelOptions options;
250+
options.protocol = brpc::PROTOCOL_HTTP;
251+
options.connection_type = brpc::CONNECTION_TYPE_POOLED;
252+
options.connection_group = connection_group;
253+
options.max_retry = 0;
254+
ASSERT_EQ(0, channel->Init(ep, &options));
255+
}
256+
257+
void CallVersion(brpc::Channel* channel, brpc::Controller* cntl) {
258+
cntl->http_request().uri() = "/status";
259+
cntl->http_request().set_method(brpc::HTTP_METHOD_GET);
260+
channel->CallMethod(NULL, cntl, NULL, NULL, NULL);
261+
}
262+
263+
void CallHttpEcho(brpc::Channel* channel, brpc::Controller* cntl) {
264+
test::EchoRequest req;
265+
test::EchoResponse res;
266+
req.set_message(EXP_REQUEST);
267+
cntl->http_request().uri() = "/EchoService/Echo";
268+
cntl->http_request().set_method(brpc::HTTP_METHOD_POST);
269+
cntl->http_request().set_content_type("application/json");
270+
channel->CallMethod(NULL, cntl, &req, &res, NULL);
271+
}
272+
228273

229274
brpc::policy::HttpContext* MakeResponseMessage(int code) {
230275
brpc::policy::HttpContext* msg = new brpc::policy::HttpContext(false);
@@ -300,6 +345,14 @@ class HttpTest : public ::testing::Test{
300345
MyAuthenticator _auth;
301346
};
302347

348+
int AllocateFreePortOrDie() {
349+
butil::fd_guard fd(tcp_listen(butil::EndPoint(butil::my_ip(), 0)));
350+
EXPECT_GE(fd, 0);
351+
butil::EndPoint point;
352+
EXPECT_EQ(0, butil::get_local_side(fd, &point));
353+
return point.port;
354+
}
355+
303356
TEST_F(HttpTest, indenting_ostream) {
304357
std::ostringstream os1;
305358
brpc::IndentingOStream is1(os1, 2);
@@ -355,6 +408,12 @@ TEST_F(HttpTest, verify_request) {
355408
}
356409
{
357410
brpc::policy::HttpContext* msg = MakeGetRequestMessage("/status");
411+
VerifyMessage(msg, false);
412+
msg->Destroy();
413+
}
414+
{
415+
brpc::policy::HttpContext* msg = MakeGetRequestMessage("/status");
416+
msg->header().SetHeader("Authorization", MOCK_CREDENTIAL);
358417
VerifyMessage(msg, true);
359418
msg->Destroy();
360419
}
@@ -379,6 +438,97 @@ TEST_F(HttpTest, verify_request) {
379438
}
380439
}
381440

441+
TEST_F(HttpTest, verify_builtin_request_on_internal_port) {
442+
_server._options.internal_port = 9527;
443+
{
444+
brpc::policy::HttpContext* msg = MakeGetRequestMessage("/status");
445+
VerifyMessage(msg, false);
446+
msg->Destroy();
447+
}
448+
{
449+
brpc::policy::HttpContext* msg = MakeGetRequestMessage("/status");
450+
VerifyMessageFromLocalPort(msg, true, _server._options.internal_port);
451+
msg->Destroy();
452+
}
453+
}
454+
455+
TEST_F(HttpTest, builtin_auth_policy_on_public_and_internal_port) {
456+
const int saved_max_connection_pool_size = brpc::FLAGS_max_connection_pool_size;
457+
brpc::FLAGS_max_connection_pool_size = 1;
458+
459+
butil::EndPoint ep;
460+
ASSERT_EQ(0, str2endpoint("127.0.0.1:0", &ep));
461+
462+
brpc::Server server;
463+
MyEchoService svc;
464+
MyAuthenticator auth;
465+
brpc::ServerOptions options;
466+
options.auth = &auth;
467+
options.internal_port = AllocateFreePortOrDie();
468+
ASSERT_EQ(0, server.AddService(&svc, brpc::SERVER_DOESNT_OWN_SERVICE));
469+
ASSERT_EQ(0, server.Start(ep, &options));
470+
ep = server.listen_address();
471+
const butil::EndPoint internal_ep(ep.ip, options.internal_port);
472+
473+
{
474+
brpc::Channel chan;
475+
brpc::ChannelOptions copt;
476+
copt.protocol = brpc::PROTOCOL_HTTP;
477+
copt.max_retry = 0;
478+
ASSERT_EQ(0, chan.Init(ep, &copt));
479+
480+
brpc::Controller cntl;
481+
cntl.http_request().uri() = "/status";
482+
cntl.http_request().set_method(brpc::HTTP_METHOD_GET);
483+
chan.CallMethod(NULL, &cntl, NULL, NULL, NULL);
484+
ASSERT_TRUE(cntl.Failed());
485+
ASSERT_EQ(brpc::EHTTP, cntl.ErrorCode()) << cntl.ErrorText();
486+
ASSERT_EQ(brpc::HTTP_STATUS_FORBIDDEN, cntl.http_response().status_code());
487+
}
488+
489+
{
490+
brpc::Channel chan;
491+
brpc::ChannelOptions copt;
492+
copt.protocol = brpc::PROTOCOL_HTTP;
493+
copt.max_retry = 0;
494+
ASSERT_EQ(0, chan.Init(internal_ep, &copt));
495+
496+
brpc::Controller cntl;
497+
cntl.http_request().uri() = "/status";
498+
cntl.http_request().set_method(brpc::HTTP_METHOD_GET);
499+
chan.CallMethod(NULL, &cntl, NULL, NULL, NULL);
500+
ASSERT_FALSE(cntl.Failed()) << cntl.ErrorText();
501+
ASSERT_EQ(brpc::HTTP_STATUS_OK, cntl.http_response().status_code());
502+
}
503+
504+
{
505+
const std::string connection_group = "builtin-auth-policy";
506+
brpc::Channel builtin_channel;
507+
brpc::Channel protected_channel;
508+
brpc::ChannelOptions copt;
509+
copt.protocol = brpc::PROTOCOL_HTTP;
510+
copt.connection_type = brpc::CONNECTION_TYPE_POOLED;
511+
copt.connection_group = connection_group;
512+
copt.max_retry = 0;
513+
ASSERT_EQ(0, builtin_channel.Init(ep, &copt));
514+
ASSERT_EQ(0, protected_channel.Init(ep, &copt));
515+
516+
brpc::Controller builtin_cntl;
517+
CallVersion(&builtin_channel, &builtin_cntl);
518+
ASSERT_TRUE(builtin_cntl.Failed());
519+
ASSERT_EQ(brpc::EHTTP, builtin_cntl.ErrorCode()) << builtin_cntl.ErrorText();
520+
ASSERT_EQ(brpc::HTTP_STATUS_FORBIDDEN, builtin_cntl.http_response().status_code());
521+
522+
brpc::Controller protected_cntl;
523+
CallHttpEcho(&protected_channel, &protected_cntl);
524+
ASSERT_TRUE(protected_cntl.Failed());
525+
}
526+
527+
ASSERT_EQ(0, server.Stop(0));
528+
ASSERT_EQ(0, server.Join());
529+
brpc::FLAGS_max_connection_pool_size = saved_max_connection_pool_size;
530+
}
531+
382532
TEST_F(HttpTest, process_request_failed_socket) {
383533
brpc::policy::HttpContext* msg = MakePostRequestMessage("/EchoService/Echo");
384534
_socket->SetFailed();

0 commit comments

Comments
 (0)