New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
openssl: Integrate Peter Wu's SSLKEYLOGFILE implementation #1866
Conversation
This is an adaptation of 2 of Peter Wu's SSLKEYLOGFILE implementations. The first one, written for old OpenSSL versions: https://git.lekensteyn.nl/peter/wireshark-notes/tree/src/sslkeylog.c The second one, written for BoringSSL and new OpenSSL versions: curl#1346 Note the first one is GPL licensed but the author gave permission to waive that license for libcurl. As of right now this feature is disabled by default, and does not have a configure option to enable it. To enable this feature define USE_CURL_SSLKEYLOGFILE when building libcurl and set environment variable SSLKEYLOGFILE to a filename that will receive the keys. And in Wireshark change your preferences to point to that key file: Edit > Preferences > Protocols > SSL > Master-Secret Co-authored-by: Peter Wu Ref: curl#1030 Ref: curl#1346 Closes curl#1866
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Note that currently you need CFLAGS=-DUSE_CURL_SSLKEYLOGFILE
(or the CMake equivalent) when building as there is no configurable option, is that intentional? (That also sounds reasonable to me.)
@@ -2325,6 +2488,11 @@ static CURLcode ossl_connect_step2(struct connectdata *conn, int sockindex) | |||
ERR_clear_error(); | |||
|
|||
err = SSL_connect(BACKEND->handle); | |||
/* If keylogging is enabled but the keylog callback is not supported then log | |||
secrets here, immediately after SSL_connect by using tap_ssl_key. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be worth noting that this increases latency (between receiving a handshake message and actually writing keys to disk) when compared to the keylog callback?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be worth noting that this increases latency (between receiving a handshake message and actually writing keys to disk) when compared to the keylog callback?
That's better left for the eventual documentation. I fixed the typo and changed the define to ENABLE_SSLKEYLOGFILE, and landed it just now. Let's continue the discussion in #1346.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I just closed that issue since that PR is superseded. Should I reopen it or just file an issue to remind of that problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's fine we can continue to talk there even though it's closed, I thought it would be easier than jump to a new thread
* Whether SSL_CTX_set_keylog_callback is available. | ||
* OpenSSL: supported since 1.1.1 https://github.com/openssl/openssl/pull/2287 | ||
* BoringSSL: supported since d28f59c27bac (committed 2015-11-19), the | ||
* BORINGSSL_201512 macro for 2016-01-21 should be close enough. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I just noticed that this "for" should be a "from"
See f083a44
This is an adaptation of 2 of Peter Wu's SSLKEYLOGFILE implementations.
The first one, written for old OpenSSL versions:
https://git.lekensteyn.nl/peter/wireshark-notes/tree/src/sslkeylog.c
The second one, written for BoringSSL and new OpenSSL versions:
#1346
Note the first one is GPL licensed but the author gave permission to
waive that license for libcurl.
As of right now this feature is disabled by default, and does not have
a configure option to enable it. To enable this feature define
USE_CURL_SSLKEYLOGFILE when building libcurl and set environment
variable SSLKEYLOGFILE to a filename that will receive the keys.
And in Wireshark change your preferences to point to that key file:
Edit > Preferences > Protocols > SSL > Master-Secret
Co-authored-by: Peter Wu
Ref: #1030
Ref: #1346
Closes #1866
A build time option that enables USE_CURL_SSLKEYLOGFILE will be added tonight or tomorrow, however I'd like to get this in now before the feature window closes, as discussed over the weekend in #1346.
/cc @Lekensteyn @bagder