Copilot commented on code in PR #3319:
URL: https://github.com/apache/brpc/pull/3319#discussion_r3359955772
##########
src/brpc/details/ssl_helper.cpp:
##########
@@ -443,6 +445,12 @@ static int SetSSLOptions(SSL_CTX* ctx, const std::string&
ciphers,
ssloptions |= SSL_OP_NO_TLSv1_2;
}
#endif // SSL_OP_NO_TLSv1_2
+
+#ifdef SSL_OP_NO_TLSv1_3
+ if (!(protocols & TLSv1_3)) {
+ ssloptions |= SSL_OP_NO_TLSv1_3;
+ }
+#endif // SSL_OP_NO_TLSv1_3
Review Comment:
Enabling TLSv1.3 via the new protocol flag means TLSv1.3 handshakes can now
happen, but the existing `ciphers` option is only applied via
SSL_CTX_set_cipher_list(), which does not control TLSv1.3 ciphersuites (OpenSSL
uses SSL_CTX_set_ciphersuites for TLSv1.3). As-is, users who set `ciphers` may
incorrectly assume it restricts TLSv1.3, which can be a compliance/security
issue. Consider exposing a dedicated TLSv1.3 ciphersuites option (and wiring it
to SSL_CTX_set_ciphersuites when available) or explicitly documenting that
`ciphers` does not affect TLSv1.3.
##########
src/brpc/details/ssl_helper.cpp:
##########
@@ -86,6 +86,8 @@ static int ParseSSLProtocols(const std::string& str_protocol)
{
protocol_flag |= TLSv1_1;
} else if (strncasecmp(protocol.data(), "TLSv1.2", protocol.size()) ==
0) {
protocol_flag |= TLSv1_2;
+ } else if (strncasecmp(protocol.data(), "TLSv1.3", protocol.size()) ==
0) {
+ protocol_flag |= TLSv1_3;
} else {
LOG(ERROR) << "Unknown SSL protocol=" << protocol;
return -1;
Review Comment:
ParseSSLProtocols uses strncasecmp(protocol.data(), "TLSv1.x",
protocol.size()) without first ensuring the token length matches the literal
length. When the token is longer than the literal being compared (e.g. token
"TLSv1.1" reaches the earlier "TLSv1" branch), strncasecmp may read past the
end of the string literal, which is undefined behavior. Please guard with an
exact-length check before calling strncasecmp (pattern already used elsewhere
in the repo).
##########
src/brpc/ssl_options.h:
##########
@@ -79,8 +79,8 @@ struct ChannelSSLOptions {
std::string ciphers;
// SSL protocols used for SSL handshake, separated by comma.
- // Available protocols: SSLv3, TLSv1, TLSv1.1, TLSv1.2
- // Default: TLSv1, TLSv1.1, TLSv1.2
+ // Available protocols: SSLv3, TLSv1, TLSv1.1, TLSv1.2, TLSv1.3
+ // Default: TLSv1, TLSv1.1, TLSv1.2, TLSv1.3
std::string protocols;
Review Comment:
The documentation for ChannelSSLOptions::protocols now states the default
includes TLSv1.3, which conflicts with the PR description's claim that TLSv1.3
is opt-in and defaults remain unchanged. If TLSv1.3 should be opt-in, keep
TLSv1.3 in the *available* list but restore the documented default to
TLSv1/TLSv1.1/TLSv1.2.
##########
src/brpc/ssl_options.cpp:
##########
@@ -27,7 +27,7 @@ VerifyOptions::VerifyOptions()
ChannelSSLOptions::ChannelSSLOptions()
: ciphers("DEFAULT")
- , protocols("TLSv1, TLSv1.1, TLSv1.2")
+ , protocols("TLSv1, TLSv1.1, TLSv1.2, TLSv1.3")
{}
Review Comment:
The PR description says TLSv1.3 support is opt-in and existing defaults
remain unchanged, but the ChannelSSLOptions default now includes "TLSv1.3".
That makes TLSv1.3 potentially negotiated by default on OpenSSL builds that
support it, which is a behavior change and contradicts the description. If
opt-in is intended, keep the default protocols list unchanged and require users
to explicitly add TLSv1.3.
--
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]
