Skip to content
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 from

Conversation

jay
Copy link
Member

@jay 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

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
@jay jay added the TLS label Sep 5, 2017
Copy link
Contributor

@Lekensteyn Lekensteyn left a 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. */
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

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?

Copy link
Member Author

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.
Copy link
Contributor

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

@jay jay closed this in 6cdba64 Sep 6, 2017
@jay jay deleted the openssl_dump_secrets_squashed branch September 6, 2017 04:03
@coveralls
Copy link

Coverage Status

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

1 similar comment
@coveralls
Copy link

Coverage Status

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

@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants