Copilot commented on code in PR #3598:
URL: https://github.com/apache/brpc/pull/3598#discussion_r4246473461
##########
test/brpc_redis_unittest.cpp:
##########
@@ -1576,6 +1576,52 @@ TEST_F(RedisTest, server_is_not_served_on_internal_port)
{
ASSERT_EQ(0, server.Join());
}
+class MyServerAuthenticator : public brpc::Authenticator {
+public:
+ int GenerateCredential(std::string*) const override { return 0; }
+ int VerifyCredential(const std::string&, const butil::EndPoint&,
+ brpc::AuthContext*) const override { return 0; }
+};
Review Comment:
This introduces another global `MyServerAuthenticator` definition; combined
with the same class name/definition added in `test/brpc_rtmp_unittest.cpp`,
this is an ODR violation that may produce duplicate symbol issues or undefined
behavior. Prefer placing test-only helpers in an anonymous namespace or using
unique per-file names.
##########
test/brpc_rtmp_unittest.cpp:
##########
@@ -1250,6 +1250,42 @@ TEST(RtmpTest,
rejected_create_stream_with_response_write_failure) {
server.Join();
}
+class MyServerAuthenticator : public brpc::Authenticator {
+public:
+ int GenerateCredential(std::string*) const override { return 0; }
+ int VerifyCredential(const std::string&, const butil::EndPoint&,
+ brpc::AuthContext*) const override { return 0; }
+};
Review Comment:
Defining `MyServerAuthenticator` in the global namespace in multiple test
translation units (it’s also introduced in `test/brpc_redis_unittest.cpp`)
violates the C++ One Definition Rule and can lead to duplicate RTTI/vtable
symbols or undefined behavior at link/runtime. Put this class in an anonymous
namespace, rename it to something file-specific, or make it a local class
inside the test.
##########
src/brpc/policy/redis_protocol.cpp:
##########
@@ -133,6 +133,15 @@ ParseResult ParseRedisMessage(butil::IOBuf* source,
Socket* socket,
// was not enabled on this port at all.
return MakeParseError(PARSE_ERROR_TRY_OTHERS);
}
+ if (server->options().auth != nullptr && !socket->IsAuthenticated()) {
+ // Redis commands are handled right here rather than in
+ // ProcessRedisRequest(), so they never reach InputMessenger's
+ // authentication hook, and the protocol carries no credential
+ // that ServerOptions.auth could verify.
+ LOG(WARNING) << "Reject redis request from unauthenticated "
+ << *socket;
Review Comment:
Logging `WARNING` on every unauthenticated Redis request can be used for log
amplification (easy DoS) and may be noisy in production if probes hit the port.
Consider rate-limiting (e.g., `LOG_EVERY_N` / `LOG_EVERY_SECOND`) or lowering
verbosity while still returning `PARSE_ERROR_ABSOLUTELY_WRONG`.
##########
src/brpc/policy/mongo_protocol.cpp:
##########
@@ -171,6 +171,17 @@ ParseResult ParseMongoMessage(butil::IOBuf* source,
return MakeMessage(msg);
}
+bool VerifyMongoRequest(const InputMessageBase* msg_base) {
+ Server* server = (Server*)msg_base->arg();
+ if (server->options().auth) {
+ // The mongo protocol carries no credential that ServerOptions.auth
+ // could verify.
+ LOG(WARNING) << "mongo does not support authentication";
Review Comment:
The warning message is ambiguous/misleading: Mongo itself supports
authentication, but this code is specifically about `ServerOptions.auth` not
being supported by the brpc mongo protocol adapter (no credential to verify).
Consider changing the text to something explicit like \"ServerOptions.auth is
not supported for mongo protocol (no credential to verify)\" and include
peer/socket info if available.
##########
src/brpc/policy/mongo_protocol.cpp:
##########
@@ -171,6 +171,17 @@ ParseResult ParseMongoMessage(butil::IOBuf* source,
return MakeMessage(msg);
}
+bool VerifyMongoRequest(const InputMessageBase* msg_base) {
+ Server* server = (Server*)msg_base->arg();
+ if (server->options().auth) {
+ // The mongo protocol carries no credential that ServerOptions.auth
+ // could verify.
+ LOG(WARNING) << "mongo does not support authentication";
+ return false;
+ }
+ return true;
+}
Review Comment:
`VerifyMongoRequest` rejects *all* requests whenever `ServerOptions.auth` is
set, even if the connection might already be authenticated by some other
mechanism that marks `socket->IsAuthenticated()` (similar to the Redis/RTMP
checks). To keep behavior consistent and avoid rejecting legitimately
authenticated sockets, gate the rejection on the socket authentication state
(e.g., reject only when `auth != nullptr` AND the socket is not authenticated).
##########
src/brpc/policy/mongo_protocol.cpp:
##########
@@ -171,6 +171,17 @@ ParseResult ParseMongoMessage(butil::IOBuf* source,
return MakeMessage(msg);
}
+bool VerifyMongoRequest(const InputMessageBase* msg_base) {
+ Server* server = (Server*)msg_base->arg();
Review Comment:
Avoid C-style casts here. Use `static_cast<Server*>(msg_base->arg())` (or an
equivalent safer cast) to make intent explicit and prevent accidental unsafe
conversions.
--
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]
