add callback to export secrets for SSLKEYLOGFILE #4043

volok-aleksej · 2024-05-06T13:31:26Z

in according with this document, i add callback to export secrets for SSLKEYLOGFILE

coveralls · 2024-05-06T15:41:25Z

coverage: 91.852% (+0.002%) from 91.85%
when pulling caa04f4 on volok-aleksej:ssl_key_log_file
into 19567e3 on randombit:master.

reneme

Thank you for taking this up! No objection to add this functionality, in general. I just added a few suggestions for simplification, pending the opinion of @randombit.

A few more general remarks:

Consider renaming TLS::Callbacks::tls_ssl_key_log_data() to ...::tls_log_secret()
The test cases in src/tests/test_tls_rfc8448.cpp are well-suited to verify this new callback; they feature some hard-coded TLS 1.3 traces as specified in RFC 8448. I can take this task, as it will likely require some additions to the test's already intricate architecture.
(Edit: I ended up extending the unit test in test_tls_cipher_state.cpp instead)
Optional: Add an example (in src/examples) that uses the callback to actually emit a file stream formatted as specified in draft-thomson-tls-keylogfile-00

src/lib/tls/tls_callbacks.h

src/lib/tls/tls_policy.h

src/lib/tls/tls_callbacks.h

src/lib/tls/tls12/tls_client_impl_12.cpp

src/lib/tls/tls12/tls_server_impl_12.cpp

src/lib/tls/tls13/tls_client_impl_13.cpp

volok-aleksej · 2024-05-08T12:31:04Z

@reneme I took all your suggestions into account. I just have to add an example. Please, review once more

volok-aleksej · 2024-05-09T12:39:38Z

I have added example. Waiting for review.

reneme · 2024-05-09T16:02:16Z

Thanks! I'm off currently on vacation. I'll do my best to review next week.

randombit

Seems fine to me to add this functionality. I left a few comments wrt documentation and the policy. @reneme has touched TLS at more more than I in recent times, so I'd like his approval on the overall change.

src/lib/tls/tls_policy.h

src/lib/tls/tls_callbacks.h

src/lib/tls/tls_policy.cpp

reneme

General Remarks and Extension

I iterated on your proposal to have Client/Server_Impl_13 derive from an abstract callback class to encapsulate the dependency on the HandshakeState and Callbacks as well as Policy. I mostly moved the definition of said class into tls_channel_impl_13.h and cleaned up some unnecessary casts.

Also, I extended the test in test_tls_cipher_state.cpp to not only mock the logging calls but also to verify that the expected secrets were logged (as they are defined in RFC8448). Please cherry-pick (and perhaps squash) my commits into this pull request: https://github.com/reneme/botan/tree/ssl_key_log_file.

Missing Functionality: Key Update

Currently, this implementation does not log secrets when a key update is performed. See section 3.1 of the draft:

Key updates (Section 7.2 of [TLS13]) result in new secrets being generated for protecting application_data records. The label used for these secrets comprises a base label of "CLIENT_TRAFFIC_SECRET_" for a client or "SERVER_TRAFFIC_SECRET_" for a server, plus the decimal value of a counter. This counter identifies the number of key updates that occurred to produce this secret. This counter starts at 0, which produces the first application data traffic secret, as above.

The key updates are handled in the Cipher_State class as well (see update_read_keys(), update_write_keys()); hence, supporting CLIENT/SERVER_TRAFFIC_SECRET_N (with N > 0) should be easily doable. Could you take a look at this @volok-aleksej?

src/examples/tls_ssl_key_log_file.cpp

fixed build of test of sslkeylogfile updated some function description reverted default value of Policy::allow_ssl_key_log_file to false

reneme

Thanks @volok-aleksej! The patch looks good to me now, except the extension of Policy::print() as detailed below.

Repeating (for future reference): The TLS 1.3 key update currently does not produce a CLIENT_TRAFFIC_SECRET_N and SERVER_TRAFFIC_SECRET_N. This could be done as future work.

src/lib/tls/tls_text_policy.cpp

volok-aleksej · 2024-05-23T11:04:18Z

Ок. I'll do it. In progress.

volok-aleksej · 2024-05-23T12:29:28Z

@reneme I've added key update logging (CLIENT_TRAFFIC_SECRET_N and SERVER_TRAFFIC_SECRET_N, where N>0), and extended Policy::print()

reneme · 2024-05-23T13:24:30Z

I've added key update logging

Nice! Thank you. I took the liberty to adapt your implementation somewhat. Please replace your last commit with this one: reneme@f3eae3c and squash your history. Then this should be ready to go.

randombit · 2024-05-23T13:41:00Z

LGTM though I also prefer @reneme's approach on the last commit

…ECRET_N where N>0) Co-Authored-By: René Meusel <[email protected]>

volok-aleksej · 2024-05-23T13:46:28Z

@reneme thanks. I've replaced last commit. Waiting for @randombit.

reneme

Awesome! Thanks a lot for your contribution.

reneme · 2024-05-23T15:19:41Z

@randombit I cannot merge, presumably because your last verdict is "changes requested". Please go ahead once you're happy with the patch.

randombit

Thanks @volok-aleksej LGTM

reneme requested changes May 7, 2024

View reviewed changes

volok-aleksej force-pushed the ssl_key_log_file branch from 7f1e5a2 to 6037524 Compare May 8, 2024 12:26

volok-aleksej requested a review from reneme May 8, 2024 12:28

volok-aleksej force-pushed the ssl_key_log_file branch from 6037524 to 6245d61 Compare May 9, 2024 12:11

randombit self-requested a review May 17, 2024 11:14

randombit requested changes May 17, 2024

View reviewed changes

src/lib/tls/tls_policy.h Show resolved Hide resolved

src/lib/tls/tls_callbacks.h Show resolved Hide resolved

src/lib/tls/tls_policy.cpp Outdated Show resolved Hide resolved

reneme requested changes May 22, 2024

View reviewed changes

src/examples/tls_ssl_key_log_file.cpp Outdated Show resolved Hide resolved

add callback to export secrets for SSLKEYLOGFILE

65dc230

volok-aleksej force-pushed the ssl_key_log_file branch from 158d0ab to 65dc230 Compare May 23, 2024 07:38

reneme and others added 4 commits May 23, 2024 10:49

Simplify Secret_Logger class somewhat

9d75f81

Test: extend tls_cipher_state test for secret logging

24196cd

Fix: rewrap some comments

c284815

some improvements

87589eb

fixed build of test of sslkeylogfile updated some function description reverted default value of Policy::allow_ssl_key_log_file to false

reneme approved these changes May 23, 2024

View reviewed changes

src/lib/tls/tls_text_policy.cpp Show resolved Hide resolved

volok-aleksej requested a review from randombit May 23, 2024 12:29

volok-aleksej force-pushed the ssl_key_log_file branch from 4ff9396 to 98d75cc Compare May 23, 2024 12:38

logging of updating keys(CLIENT_TRAFFIC_SECRET_N and SERVER_TRAFFIC_S…

caa04f4

…ECRET_N where N>0) Co-Authored-By: René Meusel <[email protected]>

volok-aleksej force-pushed the ssl_key_log_file branch from 98d75cc to caa04f4 Compare May 23, 2024 13:44

reneme approved these changes May 23, 2024

View reviewed changes

randombit approved these changes May 23, 2024

View reviewed changes

randombit merged commit 9a5a367 into randombit:master May 23, 2024
43 checks passed

volok-aleksej deleted the ssl_key_log_file branch May 23, 2024 17:02

reneme mentioned this pull request May 24, 2024

FIX: some code smells found by clang-tidy #4070

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add callback to export secrets for SSLKEYLOGFILE #4043

add callback to export secrets for SSLKEYLOGFILE #4043

volok-aleksej commented May 6, 2024

coveralls commented May 6, 2024 •

edited

Loading

reneme left a comment •

edited

Loading

volok-aleksej commented May 8, 2024

volok-aleksej commented May 9, 2024

reneme commented May 9, 2024

randombit left a comment

reneme left a comment

reneme left a comment

volok-aleksej commented May 23, 2024

volok-aleksej commented May 23, 2024

reneme commented May 23, 2024

randombit commented May 23, 2024

volok-aleksej commented May 23, 2024

reneme left a comment

reneme commented May 23, 2024

randombit left a comment

add callback to export secrets for SSLKEYLOGFILE #4043

add callback to export secrets for SSLKEYLOGFILE #4043

Conversation

volok-aleksej commented May 6, 2024

coveralls commented May 6, 2024 • edited Loading

reneme left a comment • edited Loading

Choose a reason for hiding this comment

volok-aleksej commented May 8, 2024

volok-aleksej commented May 9, 2024

reneme commented May 9, 2024

randombit left a comment

Choose a reason for hiding this comment

reneme left a comment

Choose a reason for hiding this comment

General Remarks and Extension

Missing Functionality: Key Update

reneme left a comment

Choose a reason for hiding this comment

volok-aleksej commented May 23, 2024

volok-aleksej commented May 23, 2024

reneme commented May 23, 2024

randombit commented May 23, 2024

volok-aleksej commented May 23, 2024

reneme left a comment

Choose a reason for hiding this comment

reneme commented May 23, 2024

randombit left a comment

Choose a reason for hiding this comment

coveralls commented May 6, 2024 •

edited

Loading

reneme left a comment •

edited

Loading