-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Add option to decrypt Quic traffic in debug builds #83001
Conversation
Tagging subscribers to this area: @dotnet/ncl Issue Detailsessentially
One can feed this file to Wireshark in TLS section One can inspect Quic handshake as well as the content. Wireshark offers "Follow Quic Stream" to see single H/3 (or other transaction) This does not impacts shipping product for security reasons.
|
BTW if the secret file is updated later then Wireshark deselects the traffic one needs to force that process again. Easiest way IMHO is to ignore/unignore first packet of the stream. Saving and reading from file works as well but the previous method works with life captures. |
BTW https://lekensteyn.nl/files/wireshark-ssl-tls-decryption-secrets-sharkfest18eu.pdf is great reading about how it works. |
Apart from the review, I also found this issue: microsoft/msquic#2344. I assume that because of that, this will not fully work on server side, am I right? If so, we should probably put a comment linking the issue if someone stumbles on this. |
src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicTlsSecret.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicTlsSecret.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicTlsSecret.cs
Show resolved
Hide resolved
src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicTlsSecret.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs
Outdated
Show resolved
Hide resolved
} | ||
} | ||
|
||
public void WriteSecret(string fileName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the split into 2 public methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The screen one will open file for each Quic connection and allocate. And I did not want to drag the stream to QuicConnection
right now and I did not want to worry about flushing in log running process. So I split them even if there is currently no benefit. I can merge them together but this was my motivation for keeping them separate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand. WriteSecret
is called from 1 place and the parameter-less overload is used. The body of the 2 other overloads could just be collapsed into the parameter-less one, without dragging stream anywhere.
Or did you mean for the stream to be static?
I was not aware of that. And of course I tested with client. I can perhaps look if I found some spare time. |
Should we wait until microsoft/msquic#2344 is fixed, or we believe there's value for merging for only the client side now? I'm leaning towards the latter, given that even when msquic's issue will be fixed, it would need to make its way into official release for the feature to be working everywhere... (but we need to add a comment that will mention that for a while it will work only for client) |
Server also works on Linux with microsoft/msquic#3481. On Windows, the secret file is missing client random - something one can get from packet capture. Not great but possible. |
src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicTlsSecret.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicTlsSecret.cs
Outdated
Show resolved
Hide resolved
|
||
public unsafe void WriteSecret(FileStream stream) | ||
{ | ||
lock (stream) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is here a lock?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if thaw written are really atomic so I'm worried about multiple threads writing entries concurrently for multiple connections.
} | ||
} | ||
|
||
public void WriteSecret(string fileName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand. WriteSecret
is called from 1 place and the parameter-less overload is used. The body of the 2 other overloads could just be collapsed into the parameter-less one, without dragging stream anywhere.
Or did you mean for the stream to be static?
src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicTlsSecret.cs
Outdated
Show resolved
Hide resolved
Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it. |
It took me while to get back to this. There are two big changes:
This should be ready for another review round. |
src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicTlsSecret.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicTlsSecret.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicTlsSecret.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicTlsSecret.cs
Show resolved
Hide resolved
src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicSafeHandle.cs
Show resolved
Hide resolved
src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicTlsSecret.cs
Show resolved
Hide resolved
src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicSafeHandle.cs
Show resolved
Hide resolved
I finally got some time to address feedback. This should be ready for another review round. |
src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicTlsSecret.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
essentially
One can feed this file to Wireshark in TLS section
One can inspect Quic handshake as well as the content. Wireshark offers "Follow Quic Stream" to see single H/3 (or other transaction)
This does not impacts shipping product for security reasons.
Tested only on Linux + Wireshark 4.0.4 on MacOS.