Add support for logging TLS 1.3 exporter secret by Lekensteyn · Pull Request #5702 · openssl/openssl

Lekensteyn · 2018-03-20T23:23:36Z

NSS 3.34 and boringssl have support for "EXPORTER_SECRET"
(https://bugzilla.mozilla.org/show_bug.cgi?id=1287711) which is needed
for QUIC 1-RTT decryption support in Wireshark.

Tests are updated as well. For a user, see ngtcp2/ngtcp2#67

If you are open for this, I could create another PR for EARLY_EXPORTER_SECRET which is already supported by NSS 3.35. That would be needed for QUIC 0-RTT decryption support. (Adjusting tests for this is not as straightforward as this PR though since it requires the keylog test to be invoked with early data.)

In theory Wireshark could also be modified to use the derived exporter secrets (TLS-Exporter("EXPORTER-QUIC 1rtt", "", Hash.length)), but since (1) NSS and boringssl already write this secret and (2) the TLS key log file is already a familiar mechanism in Wireshark, I think it is worth to add it to OpenSSL as well. Thoughts?

Ping @tatsuhiro-t

richsalz · 2018-03-21T01:28:24Z

The OpenSSL team will have to decide that this is a bugfix. I hope we/they do.

mattcaswell · 2018-03-21T09:29:28Z

We have already passed the feature freeze date for the 1.1.1 release. I don't think we can classify this as a bug fix although I agree it would be very useful. I see no problem including it in a post-1.1.1 release. The team as a whole would have to agree that there is sufficient justification for this to include it in 1.1.1.

mattcaswell

Approved (needs second review) - but we can't push this now due to the feature freeze

richsalz · 2018-03-21T12:01:18Z

Will you hold a vote or should I?

mattcaswell · 2018-03-21T12:38:42Z

Will you hold a vote or should I?

Go for it.

Lekensteyn · 2018-03-21T12:59:06Z

Thanks for your feedback, I have two more patches (one just extends the test suite to check for "client early traffic secret" support, another adds early "exporter secret" support + test updates). They are based off this one (in the sense that it touches the context). Should I push to this branch or create another PR?

Also, it it OK to rebase this branch on master and force-push it (to silence ASAN)?

And for some reason 1/4 appveyor build failed, due to random fuzzing or not?

mattcaswell · 2018-03-21T13:38:17Z

Thanks for your feedback, I have two more patches (one just extends the test suite to check for "client early traffic secret" support, another adds early "exporter secret" support + test updates). They are based off this one (in the sense that it touches the context). Should I push to this branch or create another PR?

I would do it all in this PR. We're going to need to vote for this to go in since we're past feature freeze. Let's vote just once instead of multiple times.

Also, it it OK to rebase this branch on master and force-push it (to silence ASAN)?

Yes, that's ok.

Lekensteyn · 2018-03-21T14:19:36Z

Rebased + added more tests + added early exporter secret.

It's not an API change, so hopefully it can be exempted from the freeze (but it is up to you) :-)

On a side note, util/openssl-format-source -v test/sslapitest.c changes indentation and spacing quite a bit, even before this PR. If you want to enforce a coding style, perhaps this could be included in CI?

mattcaswell · 2018-03-21T14:34:28Z

test/sslapitest.c

+        || !TEST_true(SSL_CTX_set_max_early_data(sctx,
+                                                 SSL3_RT_MAX_PLAIN_LENGTH))
+        || !TEST_true(SSL_CTX_set_max_early_data(cctx,
+                                                 SSL3_RT_MAX_PLAIN_LENGTH)))


It isn't necessary to set max early data on the client side. This is a server side setting.

If that is the case, then it should be removed from setupearly_data_test as well?

Yes it should. Feel free to do that here, or leave it to another PR.

richsalz · 2018-03-21T15:19:58Z

The reformat script is really not useful except as a first pass.

Lekensteyn · 2018-03-21T18:47:26Z

Found another unnecessary (but harmless) call in the tests, so removed that as well.

richsalz

Now we just need to have the vote :)

mattcaswell · 2018-03-21T20:18:52Z

Placed on hold pending the vote

richsalz · 2018-04-05T14:04:51Z

We changed the policy so that 1.3-related features can go in after the beta freeze, if three OMC members approve. We need one more @openssl/omc approval.

Lekensteyn · 2018-04-17T21:04:53Z

Hi, would it be possible to get an exemption for this patch? TLS 1.3 (and its related exporter feature) are the selling points of OpenSSL 1.1.1, but the lack of this debug feature makes it harder for IETF QUIC developers to check their implementations (powered by OpenSSL) using Wireshark.

(I just (re)discovered lack of this feature while testing with master openssl)

richsalz · 2018-04-17T21:09:13Z

Yes, we decided that TLS 1.3 features can go in during BETA. I had missed that it was reviewed, will merge now.

richsalz · 2018-04-17T21:14:08Z

Can you rebase?

NSS 3.34 and boringssl have support for "EXPORTER_SECRET" (https://bugzilla.mozilla.org/show_bug.cgi?id=1287711) which is needed for QUIC 1-RTT decryption support in Wireshark.

This will be necessary to enable Wireshark to decrypt QUIC 0-RTT data.

Client can only send early data if the PSK allows for it, the max_early_data_size field can only be configured for the server side.

Lekensteyn · 2018-04-17T22:12:53Z

Thanks for your reply. I just pushed the result of a trivial rebase, there were no conflicts and based on this diff, no further changes seemed necessary:

git log -p --stat a73d990e2b..55442b8a5b -- \
    $(git diff --name-only  693be9a2cb0fc79fe856259feea54772c18a3637..)

mattcaswell · 2018-04-17T22:18:07Z

@richsalz as per the vote we need 3 OMC members to sign off on this. At the moment we have 2.

Ping @openssl/omc.

mattcaswell · 2018-04-18T07:18:51Z

Pushed! Thanks.

NSS 3.34 and boringssl have support for "EXPORTER_SECRET" (https://bugzilla.mozilla.org/show_bug.cgi?id=1287711) which is needed for QUIC 1-RTT decryption support in Wireshark. Reviewed-by: Rich Salz <[email protected]> Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from #5702)

Reviewed-by: Rich Salz <[email protected]> Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from #5702)

This will be necessary to enable Wireshark to decrypt QUIC 0-RTT data. Reviewed-by: Rich Salz <[email protected]> Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from #5702)

Client can only send early data if the PSK allows for it, the max_early_data_size field can only be configured for the server side. Reviewed-by: Rich Salz <[email protected]> Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from #5702)

mattcaswell added issue: feature request The issue was opened to request a feature 1.1.2 labels Mar 21, 2018

mattcaswell added this to the Post 1.1.1 milestone Mar 21, 2018

mattcaswell approved these changes Mar 21, 2018

View reviewed changes

mattcaswell added the approval: review pending This pull request needs review by a committer label Mar 21, 2018

Lekensteyn mentioned this pull request Mar 21, 2018

example: Add support for writing TLS key log file ngtcp2/ngtcp2#67

Merged

Lekensteyn force-pushed the tls13-keylog-exporter-secret-only branch from e0576d8 to 0ace667 Compare March 21, 2018 14:16

mattcaswell reviewed Mar 21, 2018

View reviewed changes

mattcaswell approved these changes Mar 21, 2018

View reviewed changes

richsalz approved these changes Mar 21, 2018

View reviewed changes

richsalz added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Mar 21, 2018

mattcaswell added the hold label Mar 21, 2018

richsalz added branch: master Applies to master branch and removed hold approval: done This pull request has the required number of approvals labels Apr 5, 2018

Lekensteyn added 4 commits April 18, 2018 00:09

Add test for CLIENT_EARLY_TRAFFIC_SECRET key logging

4def496

Add support for logging early exporter secret

b43abf9

This will be necessary to enable Wireshark to decrypt QUIC 0-RTT data.

test: Remove redundant SSL_CTX_set_max_early_data

6d699c2

Client can only send early data if the PSK allows for it, the max_early_data_size field can only be configured for the server side.

Lekensteyn force-pushed the tls13-keylog-exporter-secret-only branch from 7c7fd67 to 6d699c2 Compare April 17, 2018 22:10

levitte approved these changes Apr 18, 2018

View reviewed changes

mattcaswell closed this Apr 18, 2018

levitte pushed a commit that referenced this pull request Apr 18, 2018

Add test for CLIENT_EARLY_TRAFFIC_SECRET key logging

cffe973

Reviewed-by: Rich Salz <[email protected]> Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from #5702)

Uh oh!

Comments

Conversation

Lekensteyn commented Mar 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richsalz commented Mar 21, 2018

Uh oh!

mattcaswell commented Mar 21, 2018

Uh oh!

mattcaswell left a comment

Choose a reason for hiding this comment

Uh oh!

richsalz commented Mar 21, 2018

Uh oh!

mattcaswell commented Mar 21, 2018

Uh oh!

Lekensteyn commented Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattcaswell commented Mar 21, 2018

Uh oh!

Lekensteyn commented Mar 21, 2018

Uh oh!

mattcaswell Mar 21, 2018

Choose a reason for hiding this comment

Uh oh!

Lekensteyn Mar 21, 2018

Choose a reason for hiding this comment

Uh oh!

mattcaswell Mar 21, 2018

Choose a reason for hiding this comment

Uh oh!

richsalz commented Mar 21, 2018

Uh oh!

Lekensteyn commented Mar 21, 2018

Uh oh!

richsalz left a comment

Choose a reason for hiding this comment

Uh oh!

mattcaswell commented Mar 21, 2018

Uh oh!

richsalz commented Apr 5, 2018

Uh oh!

Lekensteyn commented Apr 17, 2018

Uh oh!

richsalz commented Apr 17, 2018

Uh oh!

richsalz commented Apr 17, 2018

Uh oh!

Lekensteyn commented Apr 17, 2018

Uh oh!

mattcaswell commented Apr 17, 2018

Uh oh!

mattcaswell commented Apr 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Lekensteyn commented Mar 20, 2018 •

edited

Loading

Lekensteyn commented Mar 21, 2018 •

edited

Loading