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

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@jay
Member

jay commented Sep 5, 2017

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

openssl: Integrate Peter Wu's SSLKEYLOGFILE implementation
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

@jay jay added the SSL/TLS label Sep 5, 2017

@Lekensteyn

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. */

This comment has been minimized.

@Lekensteyn

Lekensteyn Sep 6, 2017

Member

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?

@Lekensteyn

Lekensteyn Sep 6, 2017

Member

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?

This comment has been minimized.

@jay

jay Sep 6, 2017

Member

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.

@jay

jay Sep 6, 2017

Member

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.

This comment has been minimized.

@Lekensteyn

Lekensteyn Sep 6, 2017

Member

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?

@Lekensteyn

Lekensteyn Sep 6, 2017

Member

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?

This comment has been minimized.

@jay

jay Sep 6, 2017

Member

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

@jay

jay Sep 6, 2017

Member

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.

This comment has been minimized.

@Lekensteyn

Lekensteyn Sep 6, 2017

Member

Oh, I just noticed that this "for" should be a "from"

See f083a44

@Lekensteyn

Lekensteyn Sep 6, 2017

Member

Oh, I just noticed that this "for" should be a "from"

See f083a44

@jay jay closed this in 6cdba64 Sep 6, 2017

@jay jay deleted the jay:openssl_dump_secrets_squashed branch Sep 6, 2017

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 6, 2017

Coverage Status

Coverage increased (+0.2%) to 73.296% when pulling 56accc4 on jay:openssl_dump_secrets_squashed into ee5725f on curl:master.

coveralls commented Sep 6, 2017

Coverage Status

Coverage increased (+0.2%) to 73.296% when pulling 56accc4 on jay:openssl_dump_secrets_squashed into ee5725f on curl:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 6, 2017

Coverage Status

Coverage increased (+0.2%) to 73.296% when pulling 56accc4 on jay:openssl_dump_secrets_squashed into ee5725f on curl:master.

coveralls commented Sep 6, 2017

Coverage Status

Coverage increased (+0.2%) to 73.296% when pulling 56accc4 on jay:openssl_dump_secrets_squashed into ee5725f on curl:master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment