Skip to content

Add keylog support on QUIC client#5231

Merged
masaori335 merged 1 commit intoapache:quic-latestfrom
masaori335:quic-latest-keylog
Apr 14, 2019
Merged

Add keylog support on QUIC client#5231
masaori335 merged 1 commit intoapache:quic-latestfrom
masaori335:quic-latest-keylog

Conversation

@masaori335
Copy link
Copy Markdown
Contributor

This is quite handy for debugging. Wireshark can decrypt captured QUIC packets with dumped key.
The format is NSS Key Log Format.

https://www.openssl.org/docs/man1.1.1/man3/SSL_CTX_set_keylog_callback.html
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/Key_Log_Format

But I'm not sure we should add this under iocore/net/quic/. I wanted put these code in under src/traffic_quic/, but it requires a handle to tweak SSL_CTX from there. Any ideas?

@masaori335 masaori335 added the QUIC label Apr 5, 2019
@masaori335 masaori335 added this to the QUIC milestone Apr 5, 2019
@masaori335 masaori335 self-assigned this Apr 5, 2019
Comment thread iocore/net/quic/QUICTLS_openssl.cc Outdated
@masaori335 masaori335 force-pushed the quic-latest-keylog branch from 303ef34 to 8d22a40 Compare April 9, 2019 00:29
Comment thread iocore/net/quic/QUICGlobals.cc Outdated
@maskit
Copy link
Copy Markdown
Member

maskit commented Apr 9, 2019

But I'm not sure we should add this under iocore/net/quic/. I wanted put these code in under src/traffic_quic/, but it requires a handle to tweak SSL_CTX from there. Any ideas?

I don't think we should have it in application code because the application would need to deal with a variety of SSL libraries directly. Current approach looks good to me.

Comment thread iocore/net/quic/QUICConfig.cc Outdated
Comment thread iocore/net/quic/QUICGlobals.h Outdated
{
QUICTLS *qtls = static_cast<QUICTLS *>(SSL_get_ex_data(ssl, QUIC::ssl_quic_tls_index));
const char *keylog_file = qtls->keylog_file();
std::ofstream file(keylog_file, std::ios_base::app);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How many times will this function be called? Doesn't it overwrite this file every time?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

9 times for each session. The app open mode seeks to the eos before write, iiuc.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it open and close the file 9 times, it doesn't sound great.
Also, in that case, does it mean I need to remove the file before I start new session?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it's no ideal, but it not big deal for traffic_quic. For origin server side connection or server side connection, this could be problem. But when we support it, it needs some changes to dump keys for each connections. And proxy.config.quic.client.session_file has same problem. So these should be fixed in same time and it's out of scope from this PR.

As long as you're using wireshark, you don't need to remove. It can find the correct key.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then please create an issue or put a TODO comment for the future work at least. I'm not going to stop you but I don't like leaving a trap like this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Roger.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed as #5263

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

Labels

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants