feat: enable TLS key logging via SSLKEYLOGFILE env #3173
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for the SSLKEYLOGFILE environment variable to enable TLS key logging for debugging purposes. When set, TLS master secrets are exported to a file, allowing developers to decrypt HTTPS/TLS traffic using tools like Wireshark.
Key changes:
- Implements SSLKEYLOGFILE support using pthread_once for initialization and OpenSSL's keylog callback API
- Adds compatibility guards for OpenSSL 1.1.1+ and BoringSSL
- Integrates key logging into both client and server SSL context creation
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/brpc/details/ssl_helper.cpp
Outdated
| fprintf(g_ssl_keylog_file, "%s\n", line); | ||
| fflush(g_ssl_keylog_file); |
There was a problem hiding this 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.
| 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); | ||
| } | ||
| } |
There was a problem hiding this 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.
1 participant
What problem does this PR solve?
Issue Number: resolve
Problem Summary:
This commit adds support for the
SSLKEYLOGFILEenvironment 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: