You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brpc.apache.org by "koarz (via GitHub)" <gi...@apache.org> on 2025/12/16 03:59:14 UTC
[PR] feat: enable TLS key logging via SSLKEYLOGFILE env (brpc)
koarz opened a new pull request, #3173:
URL: https://github.com/apache/brpc/pull/3173
### What problem does this PR solve?
Issue Number: resolve
Problem Summary:
This commit adds support for the `SSLKEYLOGFILE` environment variable,
enabling the export of TLS master secrets. This allows developers to
decrypt HTTPS/TLS traffic using tools like Wireshark for debugging purposes.
### What is changed and the side effects?
Changed:
Side effects:
- Performance effects:
- Breaking backward compatibility:
---
### Check List:
- Please make sure your changes are compilable.
- When providing us with a new feature, it is best to add related tests.
- Please follow [Contributor Covenant Code of Conduct](https://github.com/apache/brpc/blob/master/CODE_OF_CONDUCT.md).
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] feat: enable TLS key logging via SSLKEYLOGFILE env (brpc)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] feat: enable TLS key logging via SSLKEYLOGFILE env (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright merged PR #3173:
URL: https://github.com/apache/brpc/pull/3173
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org