Copilot commented on code in PR #3173:
URL: https://github.com/apache/brpc/pull/3173#discussion_r2621859021
##########
src/brpc/details/ssl_helper.cpp:
##########
@@ -185,6 +188,43 @@ static void SSLMessageCallback(int write_p, int version,
int content_type,
#endif // TLS1_RT_HEARTBEAT
}
+#if defined(OPENSSL_IS_BORINGSSL) || (OPENSSL_VERSION_NUMBER >= 0x10101000L)
+static pthread_once_t g_ssl_keylog_once = PTHREAD_ONCE_INIT;
+static FILE* g_ssl_keylog_file = NULL;
+
+static void InitSSLKeyLogFile() {
+ const char* path = getenv("SSLKEYLOGFILE");
+ if (path == NULL || path[0] == '\0') {
+ return;
+ }
+ g_ssl_keylog_file = fopen(path, "a");
Review Comment:
The file handle opened here is never closed, causing a resource leak. The
file remains open for the entire lifetime of the process. Consider adding a
cleanup mechanism, such as registering an atexit handler or using a RAII
wrapper to ensure the file is properly closed when the program terminates.
##########
src/brpc/details/ssl_helper.cpp:
##########
@@ -185,6 +188,43 @@ static void SSLMessageCallback(int write_p, int version,
int content_type,
#endif // TLS1_RT_HEARTBEAT
}
+#if defined(OPENSSL_IS_BORINGSSL) || (OPENSSL_VERSION_NUMBER >= 0x10101000L)
+static pthread_once_t g_ssl_keylog_once = PTHREAD_ONCE_INIT;
+static FILE* g_ssl_keylog_file = NULL;
+
+static void InitSSLKeyLogFile() {
+ const char* path = getenv("SSLKEYLOGFILE");
+ if (path == NULL || path[0] == '\0') {
+ return;
+ }
+ g_ssl_keylog_file = fopen(path, "a");
+ if (g_ssl_keylog_file == NULL) {
+ PLOG(WARNING) << "Fail to open SSLKEYLOGFILE=" << path;
+ }
+}
+
+static void SSLKeyLogCallback(const SSL* ssl, const char* line) {
+ (void)ssl;
+ if (line == NULL) {
+ return;
+ }
+ // Write the full key log line with newline in one call to keep output
atomic.
+ fprintf(g_ssl_keylog_file, "%s\n", line);
+ fflush(g_ssl_keylog_file);
+}
+
+static void MaybeSetKeyLogCallback(SSL_CTX* ctx) {
+ pthread_once(&g_ssl_keylog_once, InitSSLKeyLogFile);
+ if (ctx != NULL && g_ssl_keylog_file != NULL) {
+ SSL_CTX_set_keylog_callback(ctx, SSLKeyLogCallback);
+ }
+}
Review Comment:
The InitSSLKeyLogFile, SSLKeyLogCallback, and MaybeSetKeyLogCallback
functions lack documentation explaining their purpose, the SSLKEYLOGFILE format
expectations, and any security implications of enabling this feature. Adding
comments would help other developers understand when and how this debugging
feature should be used, especially since it exposes sensitive cryptographic
material.
##########
src/brpc/details/ssl_helper.cpp:
##########
@@ -185,6 +188,43 @@ static void SSLMessageCallback(int write_p, int version,
int content_type,
#endif // TLS1_RT_HEARTBEAT
}
+#if defined(OPENSSL_IS_BORINGSSL) || (OPENSSL_VERSION_NUMBER >= 0x10101000L)
+static pthread_once_t g_ssl_keylog_once = PTHREAD_ONCE_INIT;
+static FILE* g_ssl_keylog_file = NULL;
+
+static void InitSSLKeyLogFile() {
+ const char* path = getenv("SSLKEYLOGFILE");
+ if (path == NULL || path[0] == '\0') {
+ return;
+ }
+ g_ssl_keylog_file = fopen(path, "a");
+ if (g_ssl_keylog_file == NULL) {
+ PLOG(WARNING) << "Fail to open SSLKEYLOGFILE=" << path;
+ }
+}
+
+static void SSLKeyLogCallback(const SSL* ssl, const char* line) {
+ (void)ssl;
+ if (line == NULL) {
+ return;
+ }
+ // Write the full key log line with newline in one call to keep output
atomic.
+ fprintf(g_ssl_keylog_file, "%s\n", line);
+ fflush(g_ssl_keylog_file);
Review Comment:
The fprintf and fflush operations on g_ssl_keylog_file are not thread-safe.
The SSLKeyLogCallback can be invoked concurrently from multiple SSL
connections, and without synchronization, concurrent writes could interleave or
corrupt the output. Consider adding a mutex to protect the file write
operations or use flockfile/funlockfile for atomic writes.
##########
src/brpc/details/ssl_helper.cpp:
##########
@@ -185,6 +188,43 @@ static void SSLMessageCallback(int write_p, int version,
int content_type,
#endif // TLS1_RT_HEARTBEAT
}
+#if defined(OPENSSL_IS_BORINGSSL) || (OPENSSL_VERSION_NUMBER >= 0x10101000L)
+static pthread_once_t g_ssl_keylog_once = PTHREAD_ONCE_INIT;
+static FILE* g_ssl_keylog_file = NULL;
+
+static void InitSSLKeyLogFile() {
+ const char* path = getenv("SSLKEYLOGFILE");
+ if (path == NULL || path[0] == '\0') {
+ return;
+ }
+ g_ssl_keylog_file = fopen(path, "a");
+ if (g_ssl_keylog_file == NULL) {
+ PLOG(WARNING) << "Fail to open SSLKEYLOGFILE=" << path;
Review Comment:
The SSLKEYLOGFILE feature exposes TLS master secrets which can be used to
decrypt all traffic for those sessions. While this is intentional for
debugging, consider adding a warning log message when the feature is enabled to
alert users that sensitive cryptographic material is being written to disk,
helping prevent accidental use in production environments.
```suggestion
PLOG(WARNING) << "Fail to open SSLKEYLOGFILE=" << path;
} else {
LOG(WARNING) << "SSLKEYLOGFILE is enabled (path: " << path << "). "
<< "Sensitive TLS session keys will be written to this
file. "
<< "This feature is intended for debugging only and
should NOT be used in production environments.";
```
##########
src/brpc/details/ssl_helper.cpp:
##########
@@ -185,6 +188,43 @@ static void SSLMessageCallback(int write_p, int version,
int content_type,
#endif // TLS1_RT_HEARTBEAT
}
+#if defined(OPENSSL_IS_BORINGSSL) || (OPENSSL_VERSION_NUMBER >= 0x10101000L)
+static pthread_once_t g_ssl_keylog_once = PTHREAD_ONCE_INIT;
+static FILE* g_ssl_keylog_file = NULL;
+
+static void InitSSLKeyLogFile() {
+ const char* path = getenv("SSLKEYLOGFILE");
+ if (path == NULL || path[0] == '\0') {
+ return;
+ }
+ g_ssl_keylog_file = fopen(path, "a");
+ if (g_ssl_keylog_file == NULL) {
+ PLOG(WARNING) << "Fail to open SSLKEYLOGFILE=" << path;
+ }
+}
+
+static void SSLKeyLogCallback(const SSL* ssl, const char* line) {
+ (void)ssl;
+ if (line == NULL) {
+ return;
+ }
+ // Write the full key log line with newline in one call to keep output
atomic.
+ fprintf(g_ssl_keylog_file, "%s\n", line);
+ fflush(g_ssl_keylog_file);
Review Comment:
The SSLKeyLogCallback function performs file operations without checking if
g_ssl_keylog_file is NULL. If InitSSLKeyLogFile fails to open the file (line
200-203), g_ssl_keylog_file remains NULL, which will cause a segmentation fault
when fprintf and fflush are called with a NULL file pointer. Add a null check
before accessing g_ssl_keylog_file.
--
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]
