Support SSLKEYLOGFILE in Release builds #100665
Conversation
| return; | ||
| } | ||
|
|
||
| int totalLength = labelUtf8.Length + 1 + clientRandomUtf8.Length + 1 + 2 * secret.Length + 1; |
There was a problem hiding this comment.
Why does this need to be super optimized?
The original seemed much easier to see what will actually end up in the output:
$"CLIENT_EARLY_TRAFFIC_SECRET {clientRandom} {Convert.ToHexString(new ReadOnlySpan<byte>(_tlsSecrets->ClientEarlyTrafficSecret, _tlsSecrets->SecretLength))}\n"
There was a problem hiding this comment.
It probably doesn't, but since we are exposing it in the published product code and it is not that difficult to remove the allocation I felt it didn't hurt.
I will add a comment to make it easier to understand.
src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicRemoteExecutorTests.cs
Show resolved
Hide resolved
src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicRemoteExecutorTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicRemoteExecutorTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicRemoteExecutorTests.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Marie Píchová <11718369+ManickaP@users.noreply.github.com>
| #if DEBUG | ||
| bool isEnabled = true; | ||
| #else | ||
| bool isEnabled = AppContext.TryGetSwitch("System.Net.Security.EnableSslKeyLogging", out bool enabled) && enabled; |
There was a problem hiding this comment.
I'm wondering if we should add "Dangerous" to the name just to make it very clear.
There was a problem hiding this comment.
@dotnet/ncl What are your thoughts on this?
I was also considering removing the .Security part as this applies to Quic as well.
There was a problem hiding this comment.
- add Dangerous: seems redundant, we already have 2 levels you need to set up. Also no precedent for this with AppContext switches.
- remove .Security: I'm up for it, but I don't mind the current state either (it's still a "security" feature).
There was a problem hiding this comment.
team consensus was to not add Dangerous or other signal words and remove .Security
| @@ -21,7 +21,7 @@ static SslKeyLogger() | |||
| #if DEBUG | |||
| bool isEnabled = true; | |||
| #else | |||
| bool isEnabled = AppContext.TryGetSwitch("System.Net.Security.EnableSslKeyLogging", out bool enabled) && enabled; | |||
| bool isEnabled = AppContext.TryGetSwitch("System.Net.EnableSslKeyLogging", out bool enabled) && enabled; | |||
There was a problem hiding this comment.
While technically more accurate, we use SSL in SSLKEYLOGFILE, let's not mix another abbreviation in.
|
CI Failures are unrelated |
* Allow SSLKEYLOGFILE functionality in Release builds * Unify SSLKEYLOGFILE logging, gate behind AppContextSwitch * Add more points where QUIC secrets are logged * Enable in Debug builds without appctx switch * Update src/libraries/System.Net.Quic/src/System.Net.Quic.csproj Co-authored-by: Marie Píchová <11718369+ManickaP@users.noreply.github.com> * Code review feedback * More code review changes * i����Adjust appctx switch name --------- Co-authored-by: Marie Píchová <11718369+ManickaP@users.noreply.github.com>
Closes #37915.
This PR unifies existing SSLKEYLOGFILE exporting between SslStream and QuicConnection to use same shared code, and enables the feature in Release builds of libraries. This makes the feature available to consumers without needing to compile Debug builds of .NET Runtime.
Note that the SSLKEYLOGFILE feature is currently supported for:
*MsQuic is not yet officially supported on OSX, but once it is, no changes should be necessary from .NET side.
For security concerns, in addition to setting SSLKEYLOGFILE environment variable, setting
System.Net.EnableSslKeyLoggingAppContext switch totrueis required. This can be done either via codeor via {project}.runtimeconfig.json
Debug runtime libraires (= local development builds of .NET Runtime) don't require the AppContext switch for the feature.