You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2021/09/20 21:48:12 UTC
[GitHub] [trafficserver] bneradt opened a new pull request #8337: Adding TLS session key logging capability
bneradt opened a new pull request #8337:
URL: https://github.com/apache/trafficserver/pull/8337
Adding the ability to log TLS session keys to a log file for packet
capture decryption purposes.
This adds the following reloadable configuration:
proxy.config.ssl.keylog_file
Since this can work for QUIC as well, this also deprecates:
proxy.config.quic.client.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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on a change in pull request #8337: Adding TLS session key logging capability
Posted by GitBox <gi...@apache.org>.
zwoop commented on a change in pull request #8337:
URL: https://github.com/apache/trafficserver/pull/8337#discussion_r713087106
##########
File path: iocore/net/SSLUtils.cc
##########
@@ -100,10 +102,97 @@ static int ssl_vc_index = -1;
static ink_mutex *mutex_buf = nullptr;
static bool open_ssl_initialized = false;
-/* Using pthread thread ID and mutex functions directly, instead of
- * ATS this_ethread / ProxyMutex, so that other linked libraries
- * may use pthreads and openssl without confusing us here. (TS-2271).
- */
+TLSKeyLogger::TLSKeyLogger()
+{
Review comment:
These simple ctor's etc., why not put them in the include file where they become implicitly inlined? I'd say anything shorter than a few simple lines should go there :-).
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #8337: Adding TLS session key logging capability
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #8337:
URL: https://github.com/apache/trafficserver/pull/8337#discussion_r715829131
##########
File path: iocore/net/SSLUtils.cc
##########
@@ -100,6 +102,57 @@ static int ssl_vc_index = -1;
static ink_mutex *mutex_buf = nullptr;
static bool open_ssl_initialized = false;
+void
+TLSKeyLogger::enable_keylogging_internal(const char *keylog_file)
+{
+ Debug("ssl_keylog", "Enabling TLS key logging to: %s.", keylog_file);
+ std::unique_lock lock{_mutex};
+ if (keylog_file == nullptr) {
+ _is_enabled = false;
+ Debug("ssl_keylog", "Received a nullptr for keylog_file: disabling keylogging.");
+ return;
+ }
+
+ _filestream = std::ofstream(keylog_file, std::ios_base::app);
+ if (!_filestream.is_open()) {
+ _is_enabled = false;
+ Error("Could not open keylog file: %s", keylog_file);
+ return;
+ }
+ _is_enabled = true;
+ Note("Opened %s for TLS key logging.", keylog_file);
+}
+
+void
+TLSKeyLogger::disable_keylogging_internal()
+{
+ std::unique_lock lock{_mutex};
+ if (_is_enabled) {
+ Note("Disabling TLS key logging.");
+ }
+ _is_enabled = false;
+ _filestream = std::ofstream{};
+
+ Debug("ssl_keylog", "TLS keylogging is disabled.");
+}
+
+void
+TLSKeyLogger::log(const char *line)
+{
+ std::shared_lock lock{_mutex};
+ if (!_is_enabled) {
Review comment:
Minor point, but you'd do better to check `instance()._filename.is_open()`.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt merged pull request #8337: Adding TLS session key logging capability
Posted by GitBox <gi...@apache.org>.
bneradt merged pull request #8337:
URL: https://github.com/apache/trafficserver/pull/8337
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #8337: Adding TLS session key logging capability
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #8337:
URL: https://github.com/apache/trafficserver/pull/8337#discussion_r715829131
##########
File path: iocore/net/SSLUtils.cc
##########
@@ -100,6 +102,57 @@ static int ssl_vc_index = -1;
static ink_mutex *mutex_buf = nullptr;
static bool open_ssl_initialized = false;
+void
+TLSKeyLogger::enable_keylogging_internal(const char *keylog_file)
+{
+ Debug("ssl_keylog", "Enabling TLS key logging to: %s.", keylog_file);
+ std::unique_lock lock{_mutex};
+ if (keylog_file == nullptr) {
+ _is_enabled = false;
+ Debug("ssl_keylog", "Received a nullptr for keylog_file: disabling keylogging.");
+ return;
+ }
+
+ _filestream = std::ofstream(keylog_file, std::ios_base::app);
+ if (!_filestream.is_open()) {
+ _is_enabled = false;
+ Error("Could not open keylog file: %s", keylog_file);
+ return;
+ }
+ _is_enabled = true;
+ Note("Opened %s for TLS key logging.", keylog_file);
+}
+
+void
+TLSKeyLogger::disable_keylogging_internal()
+{
+ std::unique_lock lock{_mutex};
+ if (_is_enabled) {
+ Note("Disabling TLS key logging.");
+ }
+ _is_enabled = false;
+ _filestream = std::ofstream{};
+
+ Debug("ssl_keylog", "TLS keylogging is disabled.");
+}
+
+void
+TLSKeyLogger::log(const char *line)
+{
+ std::shared_lock lock{_mutex};
+ if (!_is_enabled) {
Review comment:
Minor point, but you'd do better to check `instance()._filename.good()`.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on pull request #8337: Adding TLS session key logging capability
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on pull request #8337:
URL: https://github.com/apache/trafficserver/pull/8337#issuecomment-926875954
I'd be tempted to change the C++ stream logic to use `open` and `writev`. The man page claims a single `writev` will always generate contiguous output even if concurrent. `writev` also avoids the copy & allocation to put a newline at the end without giving up on the contiguous output with only a read lock.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a change in pull request #8337: Adding TLS session key logging capability
Posted by GitBox <gi...@apache.org>.
bneradt commented on a change in pull request #8337:
URL: https://github.com/apache/trafficserver/pull/8337#discussion_r713405297
##########
File path: iocore/net/SSLUtils.cc
##########
@@ -100,10 +102,97 @@ static int ssl_vc_index = -1;
static ink_mutex *mutex_buf = nullptr;
static bool open_ssl_initialized = false;
-/* Using pthread thread ID and mutex functions directly, instead of
- * ATS this_ethread / ProxyMutex, so that other linked libraries
- * may use pthreads and openssl without confusing us here. (TS-2271).
- */
+TLSKeyLogger::TLSKeyLogger()
+{
Review comment:
I talked with Leif offline. He correctly points out that it's more consistent with the rest of the code base to have these smaller function definitions in the header. In keeping with this pattern, I moved these over to the header. 👍
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bryancall commented on pull request #8337: Adding TLS session key logging capability
Posted by GitBox <gi...@apache.org>.
bryancall commented on pull request #8337:
URL: https://github.com/apache/trafficserver/pull/8337#issuecomment-923418443
@SolidWallOfCode is going to look at it
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bryancall commented on pull request #8337: Adding TLS session key logging capability
Posted by GitBox <gi...@apache.org>.
bryancall commented on pull request #8337:
URL: https://github.com/apache/trafficserver/pull/8337#issuecomment-933928160
@SolidWallOfCode is going to review it again
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a change in pull request #8337: Adding TLS session key logging capability
Posted by GitBox <gi...@apache.org>.
bneradt commented on a change in pull request #8337:
URL: https://github.com/apache/trafficserver/pull/8337#discussion_r717726042
##########
File path: iocore/net/SSLUtils.cc
##########
@@ -1599,6 +1672,15 @@ SSLMultiCertConfigLoader::_set_alpn_callback(SSL_CTX *ctx)
return true;
}
+bool
+SSLMultiCertConfigLoader::_set_keylog_callback(SSL_CTX *ctx)
+{
+#if OPENSSL_VERSION_NUMBER >= 0x010100000
Review comment:
👍
Good suggestion. I added the check to the configure mechanism as suggested.
##########
File path: iocore/net/P_SSLUtils.h
##########
@@ -60,6 +62,105 @@ struct SSLLoadingContext {
explicit SSLLoadingContext(SSL_CTX *c, SSLCertContextType ctx_type) : ctx(c), ctx_type(ctx_type) {}
};
+/** A class for handling TLS secrets logging. */
+class TLSKeyLogger
+{
+public:
+ TLSKeyLogger(const TLSKeyLogger &) = delete;
+ TLSKeyLogger &operator=(const TLSKeyLogger &) = delete;
+
+ ~TLSKeyLogger()
+ {
+ std::unique_lock lock{_mutex};
+ close_keylog_file();
+ }
+
+ /** A callback for TLS secret key logging.
+ *
+ * This is the callback registered with OpenSSL's SSL_CTX_set_keylog_callback
+ * to log TLS secrets if the user enabled that feature. For more information
+ * about this callback, see OpenSSL's documentation of
+ * SSL_CTX_set_keylog_callback.
+ *
+ * @param[in] ssl The SSL object associated with the connection.
+ * @param[in] line The line to place in the keylog file.
+ */
+ static void
+ ssl_keylog_cb(const SSL *ssl, const char *line)
+ {
+ instance().log(line);
+ }
+
+ /** Return whether TLS key logging is enabled.
+ *
+ * @return True if TLS session key logging is enabled, false otherwise.
+ */
+ static bool
+ is_enabled()
+ {
+ return instance()._fd >= 0;
+ }
+
+ /** Enable keylogging.
+ *
+ * @param[in] keylog_file The path to the file to log TLS secrets to.
+ */
+ static void
+ enable_keylogging(const char *keylog_file)
+ {
+ instance().enable_keylogging_internal(keylog_file);
+ }
+
+ /** Disable TLS secrets logging. */
+ static void
+ disable_keylogging()
+ {
+ instance().disable_keylogging_internal();
+ }
+
+private:
+ TLSKeyLogger() = default;
+
+ /** Return the TLSKeyLogger singleton.
+ *
+ * We use a getter rather than a class static singleton member so that the
+ * construction of the singleton delayed until after TLS configuration is
+ * processed.
+ */
+ static TLSKeyLogger &
+ instance()
+ {
+ static TLSKeyLogger instance;
+ return instance;
+ }
+
+ /** Close the file descriptor for the key log file.
+ *
+ * @note This assumes that a unique lock has been acquired for _mutex.
+ */
+ void close_keylog_file();
+
+ /** A TLS secret line to log to the keylog file.
+ *
+ * @param[in] line A line to log to the keylog file.
+ */
+ void log(const char *line);
+
+ /** Enable TLS keylogging in the instance singleton. */
+ void enable_keylogging_internal(const char *keylog_file);
+
+ /** Disable TLS keylogging in the instance singleton. */
+ void disable_keylogging_internal();
+
+private:
Review comment:
Yes, for organizational purposes: one private is for the private functions, the other for the private variables.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a change in pull request #8337: Adding TLS session key logging capability
Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #8337:
URL: https://github.com/apache/trafficserver/pull/8337#discussion_r717112143
##########
File path: iocore/net/SSLUtils.cc
##########
@@ -1599,6 +1672,15 @@ SSLMultiCertConfigLoader::_set_alpn_callback(SSL_CTX *ctx)
return true;
}
+bool
+SSLMultiCertConfigLoader::_set_keylog_callback(SSL_CTX *ctx)
+{
+#if OPENSSL_VERSION_NUMBER >= 0x010100000
Review comment:
As we talked on Slack, I'd check availability of the function in configure script. That way should always work for any versions and also BoringSSL.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a change in pull request #8337: Adding TLS session key logging capability
Posted by GitBox <gi...@apache.org>.
bneradt commented on a change in pull request #8337:
URL: https://github.com/apache/trafficserver/pull/8337#discussion_r713325295
##########
File path: iocore/net/SSLClientUtils.cc
##########
@@ -234,6 +235,12 @@ SSLInitClientContext(const SSLConfigParams *params)
SSL_CTX_sess_set_new_cb(client_ctx, ssl_new_session_callback);
}
+ if (TLSKeyLogger::is_enabled()) {
+#if OPENSSL_VERSION_NUMBER >= 0x010100000
+ SSL_CTX_set_keylog_callback(client_ctx, TLSKeyLogger::ssl_keylog_cb);
Review comment:
Both good suggestions.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #8337: Adding TLS session key logging capability
Posted by GitBox <gi...@apache.org>.
bneradt commented on pull request #8337:
URL: https://github.com/apache/trafficserver/pull/8337#issuecomment-926943555
> I'd be tempted to change the C++ stream logic to use `open` and `writev`. The man page claims a single `writev` will always generate contiguous output even if concurrent. `writev` also avoids the copy & allocation to put a newline at the end without giving up on the contiguous output with only a read lock.
Sounds good. I changed this to use `writev`.
Thank you for the review and suggestions.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on pull request #8337: Adding TLS session key logging capability
Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #8337:
URL: https://github.com/apache/trafficserver/pull/8337#issuecomment-937882173
Cherry-picked to v9.2.x
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a change in pull request #8337: Adding TLS session key logging capability
Posted by GitBox <gi...@apache.org>.
bneradt commented on a change in pull request #8337:
URL: https://github.com/apache/trafficserver/pull/8337#discussion_r716801879
##########
File path: iocore/net/quic/QUICConfig.cc
##########
@@ -82,8 +82,13 @@ quic_init_client_ssl_ctx(const QUICConfigParams *params)
SSL_CTX_sess_set_new_cb(ssl_ctx.get(), QUIC::ssl_client_new_session);
}
+ // TODO: QUIC-specific keylogging is deprecated in favor of the more broad
Review comment:
Thanks for pointing this out. I hadn't noticed that. I agree that it would be preferable to just remove it now rather than needing to remember to remove it later. I'll remove it with this PR.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #8337: Adding TLS session key logging capability
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #8337:
URL: https://github.com/apache/trafficserver/pull/8337#discussion_r715829833
##########
File path: iocore/net/SSLUtils.cc
##########
@@ -100,6 +102,57 @@ static int ssl_vc_index = -1;
static ink_mutex *mutex_buf = nullptr;
static bool open_ssl_initialized = false;
+void
+TLSKeyLogger::enable_keylogging_internal(const char *keylog_file)
+{
+ Debug("ssl_keylog", "Enabling TLS key logging to: %s.", keylog_file);
+ std::unique_lock lock{_mutex};
+ if (keylog_file == nullptr) {
+ _is_enabled = false;
+ Debug("ssl_keylog", "Received a nullptr for keylog_file: disabling keylogging.");
+ return;
+ }
+
+ _filestream = std::ofstream(keylog_file, std::ios_base::app);
+ if (!_filestream.is_open()) {
+ _is_enabled = false;
+ Error("Could not open keylog file: %s", keylog_file);
+ return;
+ }
+ _is_enabled = true;
+ Note("Opened %s for TLS key logging.", keylog_file);
+}
+
+void
+TLSKeyLogger::disable_keylogging_internal()
+{
+ std::unique_lock lock{_mutex};
+ if (_is_enabled) {
+ Note("Disabling TLS key logging.");
+ }
+ _is_enabled = false;
+ _filestream = std::ofstream{};
+
+ Debug("ssl_keylog", "TLS keylogging is disabled.");
+}
+
+void
+TLSKeyLogger::log(const char *line)
+{
+ std::shared_lock lock{_mutex};
+ if (!_is_enabled) {
+ return;
+ }
+
+ std::stringstream terminated_line;
Review comment:
What's the point of this? Why not write `std::endl` to `instance()._filestream` and not have `terminated_line` at all?
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #8337: Adding TLS session key logging capability
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #8337:
URL: https://github.com/apache/trafficserver/pull/8337#discussion_r715831663
##########
File path: iocore/net/SSLUtils.cc
##########
@@ -100,6 +102,57 @@ static int ssl_vc_index = -1;
static ink_mutex *mutex_buf = nullptr;
static bool open_ssl_initialized = false;
+void
+TLSKeyLogger::enable_keylogging_internal(const char *keylog_file)
+{
+ Debug("ssl_keylog", "Enabling TLS key logging to: %s.", keylog_file);
+ std::unique_lock lock{_mutex};
+ if (keylog_file == nullptr) {
+ _is_enabled = false;
+ Debug("ssl_keylog", "Received a nullptr for keylog_file: disabling keylogging.");
+ return;
+ }
+
+ _filestream = std::ofstream(keylog_file, std::ios_base::app);
+ if (!_filestream.is_open()) {
+ _is_enabled = false;
+ Error("Could not open keylog file: %s", keylog_file);
+ return;
+ }
+ _is_enabled = true;
+ Note("Opened %s for TLS key logging.", keylog_file);
+}
+
+void
+TLSKeyLogger::disable_keylogging_internal()
+{
+ std::unique_lock lock{_mutex};
+ if (_is_enabled) {
+ Note("Disabling TLS key logging.");
+ }
+ _is_enabled = false;
+ _filestream = std::ofstream{};
+
+ Debug("ssl_keylog", "TLS keylogging is disabled.");
+}
+
+void
+TLSKeyLogger::log(const char *line)
+{
+ std::shared_lock lock{_mutex};
+ if (!_is_enabled) {
+ return;
+ }
+
+ std::stringstream terminated_line;
+ terminated_line << line << std::endl;
+
+ // Writing the single, fully formed string to the file stream avoids
+ // concurrency issues with multiple writers to the keylog file.
+ instance()._filestream << terminated_line.str();
Review comment:
I think there's a general problem here in that "Concurrent access to the same stream object may cause data races".
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a change in pull request #8337: Adding TLS session key logging capability
Posted by GitBox <gi...@apache.org>.
bneradt commented on a change in pull request #8337:
URL: https://github.com/apache/trafficserver/pull/8337#discussion_r717726042
##########
File path: iocore/net/SSLUtils.cc
##########
@@ -1599,6 +1672,15 @@ SSLMultiCertConfigLoader::_set_alpn_callback(SSL_CTX *ctx)
return true;
}
+bool
+SSLMultiCertConfigLoader::_set_keylog_callback(SSL_CTX *ctx)
+{
+#if OPENSSL_VERSION_NUMBER >= 0x010100000
Review comment:
👍
Good suggestion. I added the check to the configure mechanism as suggested.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on pull request #8337: Adding TLS session key logging capability
Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #8337:
URL: https://github.com/apache/trafficserver/pull/8337#issuecomment-937882173
Cherry-picked to v9.2.x
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a change in pull request #8337: Adding TLS session key logging capability
Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #8337:
URL: https://github.com/apache/trafficserver/pull/8337#discussion_r716288458
##########
File path: iocore/net/SSLUtils.cc
##########
@@ -1599,6 +1672,15 @@ SSLMultiCertConfigLoader::_set_alpn_callback(SSL_CTX *ctx)
return true;
}
+bool
+SSLMultiCertConfigLoader::_set_keylog_callback(SSL_CTX *ctx)
+{
+#if OPENSSL_VERSION_NUMBER >= 0x010100000
Review comment:
`OPENSSL_VERSION_NUMBER` is deprecated since OpenSSL 3.0. Let's not use it for new code.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a change in pull request #8337: Adding TLS session key logging capability
Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #8337:
URL: https://github.com/apache/trafficserver/pull/8337#discussion_r716293227
##########
File path: iocore/net/quic/QUICConfig.cc
##########
@@ -82,8 +82,13 @@ quic_init_client_ssl_ctx(const QUICConfigParams *params)
SSL_CTX_sess_set_new_cb(ssl_ctx.get(), QUIC::ssl_client_new_session);
}
+ // TODO: QUIC-specific keylogging is deprecated in favor of the more broad
Review comment:
Although the removal doesn't have to be made on this PR, I don't think it have to wait for a future release. There is a disclaimer below on the docs. At least I don't mind removing it immediately.
> All configurations for QUIC are still experimental and may be changed or
removed in the future without prior notice.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a change in pull request #8337: Adding TLS session key logging capability
Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #8337:
URL: https://github.com/apache/trafficserver/pull/8337#discussion_r717112143
##########
File path: iocore/net/SSLUtils.cc
##########
@@ -1599,6 +1672,15 @@ SSLMultiCertConfigLoader::_set_alpn_callback(SSL_CTX *ctx)
return true;
}
+bool
+SSLMultiCertConfigLoader::_set_keylog_callback(SSL_CTX *ctx)
+{
+#if OPENSSL_VERSION_NUMBER >= 0x010100000
Review comment:
As we talked on Slack, I'd check availability of the function in configure script. That way should always work for any versions and also BoringSSL.
##########
File path: iocore/net/P_SSLUtils.h
##########
@@ -60,6 +62,105 @@ struct SSLLoadingContext {
explicit SSLLoadingContext(SSL_CTX *c, SSLCertContextType ctx_type) : ctx(c), ctx_type(ctx_type) {}
};
+/** A class for handling TLS secrets logging. */
+class TLSKeyLogger
+{
+public:
+ TLSKeyLogger(const TLSKeyLogger &) = delete;
+ TLSKeyLogger &operator=(const TLSKeyLogger &) = delete;
+
+ ~TLSKeyLogger()
+ {
+ std::unique_lock lock{_mutex};
+ close_keylog_file();
+ }
+
+ /** A callback for TLS secret key logging.
+ *
+ * This is the callback registered with OpenSSL's SSL_CTX_set_keylog_callback
+ * to log TLS secrets if the user enabled that feature. For more information
+ * about this callback, see OpenSSL's documentation of
+ * SSL_CTX_set_keylog_callback.
+ *
+ * @param[in] ssl The SSL object associated with the connection.
+ * @param[in] line The line to place in the keylog file.
+ */
+ static void
+ ssl_keylog_cb(const SSL *ssl, const char *line)
+ {
+ instance().log(line);
+ }
+
+ /** Return whether TLS key logging is enabled.
+ *
+ * @return True if TLS session key logging is enabled, false otherwise.
+ */
+ static bool
+ is_enabled()
+ {
+ return instance()._fd >= 0;
+ }
+
+ /** Enable keylogging.
+ *
+ * @param[in] keylog_file The path to the file to log TLS secrets to.
+ */
+ static void
+ enable_keylogging(const char *keylog_file)
+ {
+ instance().enable_keylogging_internal(keylog_file);
+ }
+
+ /** Disable TLS secrets logging. */
+ static void
+ disable_keylogging()
+ {
+ instance().disable_keylogging_internal();
+ }
+
+private:
+ TLSKeyLogger() = default;
+
+ /** Return the TLSKeyLogger singleton.
+ *
+ * We use a getter rather than a class static singleton member so that the
+ * construction of the singleton delayed until after TLS configuration is
+ * processed.
+ */
+ static TLSKeyLogger &
+ instance()
+ {
+ static TLSKeyLogger instance;
+ return instance;
+ }
+
+ /** Close the file descriptor for the key log file.
+ *
+ * @note This assumes that a unique lock has been acquired for _mutex.
+ */
+ void close_keylog_file();
+
+ /** A TLS secret line to log to the keylog file.
+ *
+ * @param[in] line A line to log to the keylog file.
+ */
+ void log(const char *line);
+
+ /** Enable TLS keylogging in the instance singleton. */
+ void enable_keylogging_internal(const char *keylog_file);
+
+ /** Disable TLS keylogging in the instance singleton. */
+ void disable_keylogging_internal();
+
+private:
Review comment:
There are two `private` sections. Is it intentional?
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] randall commented on pull request #8337: Adding TLS session key logging capability
Posted by GitBox <gi...@apache.org>.
randall commented on pull request #8337:
URL: https://github.com/apache/trafficserver/pull/8337#issuecomment-926745837
[approve ci autest]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a change in pull request #8337: Adding TLS session key logging capability
Posted by GitBox <gi...@apache.org>.
bneradt commented on a change in pull request #8337:
URL: https://github.com/apache/trafficserver/pull/8337#discussion_r713337002
##########
File path: iocore/net/SSLUtils.cc
##########
@@ -100,10 +102,97 @@ static int ssl_vc_index = -1;
static ink_mutex *mutex_buf = nullptr;
static bool open_ssl_initialized = false;
-/* Using pthread thread ID and mutex functions directly, instead of
- * ATS this_ethread / ProxyMutex, so that other linked libraries
- * may use pthreads and openssl without confusing us here. (TS-2271).
- */
+TLSKeyLogger::TLSKeyLogger()
+{
Review comment:
My coding practice is pretty purist when it comes to implementation being in .cc files. It's just a preference thing for me. Also, if it's in the header, changes often impact multiple translation units which increases build times.
But that's clearly just a personal preference of mine. If you ask again I'll put it in the header. Note that since this is a singleton, this constructor is only called once.
Thanks for reviewing and sharing your thoughts.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a change in pull request #8337: Adding TLS session key logging capability
Posted by GitBox <gi...@apache.org>.
bneradt commented on a change in pull request #8337:
URL: https://github.com/apache/trafficserver/pull/8337#discussion_r717727650
##########
File path: iocore/net/P_SSLUtils.h
##########
@@ -60,6 +62,105 @@ struct SSLLoadingContext {
explicit SSLLoadingContext(SSL_CTX *c, SSLCertContextType ctx_type) : ctx(c), ctx_type(ctx_type) {}
};
+/** A class for handling TLS secrets logging. */
+class TLSKeyLogger
+{
+public:
+ TLSKeyLogger(const TLSKeyLogger &) = delete;
+ TLSKeyLogger &operator=(const TLSKeyLogger &) = delete;
+
+ ~TLSKeyLogger()
+ {
+ std::unique_lock lock{_mutex};
+ close_keylog_file();
+ }
+
+ /** A callback for TLS secret key logging.
+ *
+ * This is the callback registered with OpenSSL's SSL_CTX_set_keylog_callback
+ * to log TLS secrets if the user enabled that feature. For more information
+ * about this callback, see OpenSSL's documentation of
+ * SSL_CTX_set_keylog_callback.
+ *
+ * @param[in] ssl The SSL object associated with the connection.
+ * @param[in] line The line to place in the keylog file.
+ */
+ static void
+ ssl_keylog_cb(const SSL *ssl, const char *line)
+ {
+ instance().log(line);
+ }
+
+ /** Return whether TLS key logging is enabled.
+ *
+ * @return True if TLS session key logging is enabled, false otherwise.
+ */
+ static bool
+ is_enabled()
+ {
+ return instance()._fd >= 0;
+ }
+
+ /** Enable keylogging.
+ *
+ * @param[in] keylog_file The path to the file to log TLS secrets to.
+ */
+ static void
+ enable_keylogging(const char *keylog_file)
+ {
+ instance().enable_keylogging_internal(keylog_file);
+ }
+
+ /** Disable TLS secrets logging. */
+ static void
+ disable_keylogging()
+ {
+ instance().disable_keylogging_internal();
+ }
+
+private:
+ TLSKeyLogger() = default;
+
+ /** Return the TLSKeyLogger singleton.
+ *
+ * We use a getter rather than a class static singleton member so that the
+ * construction of the singleton delayed until after TLS configuration is
+ * processed.
+ */
+ static TLSKeyLogger &
+ instance()
+ {
+ static TLSKeyLogger instance;
+ return instance;
+ }
+
+ /** Close the file descriptor for the key log file.
+ *
+ * @note This assumes that a unique lock has been acquired for _mutex.
+ */
+ void close_keylog_file();
+
+ /** A TLS secret line to log to the keylog file.
+ *
+ * @param[in] line A line to log to the keylog file.
+ */
+ void log(const char *line);
+
+ /** Enable TLS keylogging in the instance singleton. */
+ void enable_keylogging_internal(const char *keylog_file);
+
+ /** Disable TLS keylogging in the instance singleton. */
+ void disable_keylogging_internal();
+
+private:
Review comment:
Yes, for organizational purposes: one private is for the private functions, the other for the private variables.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a change in pull request #8337: Adding TLS session key logging capability
Posted by GitBox <gi...@apache.org>.
bneradt commented on a change in pull request #8337:
URL: https://github.com/apache/trafficserver/pull/8337#discussion_r716820262
##########
File path: iocore/net/SSLUtils.cc
##########
@@ -1599,6 +1672,15 @@ SSLMultiCertConfigLoader::_set_alpn_callback(SSL_CTX *ctx)
return true;
}
+bool
+SSLMultiCertConfigLoader::_set_keylog_callback(SSL_CTX *ctx)
+{
+#if OPENSSL_VERSION_NUMBER >= 0x010100000
Review comment:
I don't mind changing to something else, but don't we need to use `OPENSSL_VERSION_NUMBER` as long as we support OpenSSL 1.1.x? Users building with OpenSSL 1.1.1 won't have access to the newer macros/functions.
Maybe I'm missing something.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a change in pull request #8337: Adding TLS session key logging capability
Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #8337:
URL: https://github.com/apache/trafficserver/pull/8337#discussion_r717143291
##########
File path: iocore/net/P_SSLUtils.h
##########
@@ -60,6 +62,105 @@ struct SSLLoadingContext {
explicit SSLLoadingContext(SSL_CTX *c, SSLCertContextType ctx_type) : ctx(c), ctx_type(ctx_type) {}
};
+/** A class for handling TLS secrets logging. */
+class TLSKeyLogger
+{
+public:
+ TLSKeyLogger(const TLSKeyLogger &) = delete;
+ TLSKeyLogger &operator=(const TLSKeyLogger &) = delete;
+
+ ~TLSKeyLogger()
+ {
+ std::unique_lock lock{_mutex};
+ close_keylog_file();
+ }
+
+ /** A callback for TLS secret key logging.
+ *
+ * This is the callback registered with OpenSSL's SSL_CTX_set_keylog_callback
+ * to log TLS secrets if the user enabled that feature. For more information
+ * about this callback, see OpenSSL's documentation of
+ * SSL_CTX_set_keylog_callback.
+ *
+ * @param[in] ssl The SSL object associated with the connection.
+ * @param[in] line The line to place in the keylog file.
+ */
+ static void
+ ssl_keylog_cb(const SSL *ssl, const char *line)
+ {
+ instance().log(line);
+ }
+
+ /** Return whether TLS key logging is enabled.
+ *
+ * @return True if TLS session key logging is enabled, false otherwise.
+ */
+ static bool
+ is_enabled()
+ {
+ return instance()._fd >= 0;
+ }
+
+ /** Enable keylogging.
+ *
+ * @param[in] keylog_file The path to the file to log TLS secrets to.
+ */
+ static void
+ enable_keylogging(const char *keylog_file)
+ {
+ instance().enable_keylogging_internal(keylog_file);
+ }
+
+ /** Disable TLS secrets logging. */
+ static void
+ disable_keylogging()
+ {
+ instance().disable_keylogging_internal();
+ }
+
+private:
+ TLSKeyLogger() = default;
+
+ /** Return the TLSKeyLogger singleton.
+ *
+ * We use a getter rather than a class static singleton member so that the
+ * construction of the singleton delayed until after TLS configuration is
+ * processed.
+ */
+ static TLSKeyLogger &
+ instance()
+ {
+ static TLSKeyLogger instance;
+ return instance;
+ }
+
+ /** Close the file descriptor for the key log file.
+ *
+ * @note This assumes that a unique lock has been acquired for _mutex.
+ */
+ void close_keylog_file();
+
+ /** A TLS secret line to log to the keylog file.
+ *
+ * @param[in] line A line to log to the keylog file.
+ */
+ void log(const char *line);
+
+ /** Enable TLS keylogging in the instance singleton. */
+ void enable_keylogging_internal(const char *keylog_file);
+
+ /** Disable TLS keylogging in the instance singleton. */
+ void disable_keylogging_internal();
+
+private:
Review comment:
There are two `private` sections. Is it intentional?
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on a change in pull request #8337: Adding TLS session key logging capability
Posted by GitBox <gi...@apache.org>.
zwoop commented on a change in pull request #8337:
URL: https://github.com/apache/trafficserver/pull/8337#discussion_r713085335
##########
File path: iocore/net/SSLClientUtils.cc
##########
@@ -234,6 +235,12 @@ SSLInitClientContext(const SSLConfigParams *params)
SSL_CTX_sess_set_new_cb(client_ctx, ssl_new_session_callback);
}
+ if (TLSKeyLogger::is_enabled()) {
+#if OPENSSL_VERSION_NUMBER >= 0x010100000
+ SSL_CTX_set_keylog_callback(client_ctx, TLSKeyLogger::ssl_keylog_cb);
Review comment:
Why not put the conditional inside the OPENSSL_VERSION_NUMBER check? And probably an "unlikely" as well ?
##########
File path: iocore/net/SSLUtils.cc
##########
@@ -100,10 +102,97 @@ static int ssl_vc_index = -1;
static ink_mutex *mutex_buf = nullptr;
static bool open_ssl_initialized = false;
-/* Using pthread thread ID and mutex functions directly, instead of
- * ATS this_ethread / ProxyMutex, so that other linked libraries
- * may use pthreads and openssl without confusing us here. (TS-2271).
- */
+TLSKeyLogger::TLSKeyLogger()
+{
Review comment:
These simple ctor's etc., why not put them in the include file where they become implicitly inlined? I'd say anything short than a few simple lines should go there :-).
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org