From cca6238c951a4d06bcb9d309bead1e84597e9703 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Fri, 5 Apr 2024 08:50:02 +0200 Subject: [PATCH 1/8] Allow SSLKEYLOGFILE functionality in Release builds --- .../Interop.OpenSsl.cs | 6 ------ .../src/System/Net/Quic/QuicConnection.cs | 8 -------- .../tests/FunctionalTests/MsQuicRemoteExecutorTests.cs | 5 ----- .../FunctionalTests/SslStreamRemoteExecutorTests.cs | 9 ++------- 4 files changed, 2 insertions(+), 26 deletions(-) diff --git a/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs b/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs index 310981b194d0e1..a1312c531c40bc 100644 --- a/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs +++ b/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs @@ -21,10 +21,8 @@ internal static partial class Interop { internal static partial class OpenSsl { -#if DEBUG private static readonly string? s_keyLogFile = Environment.GetEnvironmentVariable("SSLKEYLOGFILE"); private static readonly FileStream? s_fileStream = s_keyLogFile != null ? File.Open(s_keyLogFile, FileMode.Append, FileAccess.Write, FileShare.ReadWrite) : null; -#endif private const string TlsCacheSizeCtxName = "System.Net.Security.TlsCacheSize"; private const string TlsCacheSizeEnvironmentVariable = "DOTNET_SYSTEM_NET_SECURITY_TLSCACHESIZE"; private const SslProtocols FakeAlpnSslProtocol = (SslProtocols)1; // used to distinguish server sessions with ALPN @@ -209,12 +207,10 @@ internal static unsafe SafeSslContextHandle AllocateSslContext(SslAuthentication Ssl.SslCtxSetDefaultOcspCallback(sslCtx); } } -#if DEBUG if (s_fileStream != null) { Ssl.SslCtxSetKeylogCallback(sslCtx, &KeyLogCallback); } -#endif } catch { @@ -757,7 +753,6 @@ private static unsafe void RemoveSessionCallback(IntPtr ctx, IntPtr session) ctxHandle.RemoveSession(name); } -#if DEBUG [UnmanagedCallersOnly] private static unsafe void KeyLogCallback(IntPtr ssl, char* line) { @@ -773,7 +768,6 @@ private static unsafe void KeyLogCallback(IntPtr ssl, char* line) } } } -#endif private static int BioRead(SafeBioHandle bio, Span buffer, int count) { diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs index 0846543a6aee66..876ea3299e596f 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs @@ -187,13 +187,11 @@ static async ValueTask StartConnectAsync(QuicClientConnectionOpt /// private SslApplicationProtocol _negotiatedApplicationProtocol; -#if DEBUG /// /// Will contain TLS secret after CONNECTED event is received and store it into SSLKEYLOGFILE. /// MsQuic holds the underlying pointer so this object can be disposed only after connection native handle gets closed. /// private readonly MsQuicTlsSecret? _tlsSecret; -#endif /// /// The remote endpoint used for this connection. @@ -254,9 +252,7 @@ private unsafe QuicConnection() throw; } -#if DEBUG _tlsSecret = MsQuicTlsSecret.Create(_handle); -#endif } /// @@ -284,9 +280,7 @@ internal unsafe QuicConnection(QUIC_HANDLE* handle, QUIC_NEW_CONNECTION_INFO* in _remoteEndPoint = MsQuicHelpers.QuicAddrToIPEndPoint(info->RemoteAddress); _localEndPoint = MsQuicHelpers.QuicAddrToIPEndPoint(info->LocalAddress); -#if DEBUG _tlsSecret = MsQuicTlsSecret.Create(_handle); -#endif } private async ValueTask FinishConnectAsync(QuicClientConnectionOptions options, CancellationToken cancellationToken = default) @@ -510,9 +504,7 @@ private unsafe int HandleEventConnected(ref CONNECTED_DATA data) QuicAddr localAddress = MsQuicHelpers.GetMsQuicParameter(_handle, QUIC_PARAM_CONN_LOCAL_ADDRESS); _localEndPoint = MsQuicHelpers.QuicAddrToIPEndPoint(&localAddress); -#if DEBUG _tlsSecret?.WriteSecret(); -#endif if (NetEventSource.Log.IsEnabled()) { diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicRemoteExecutorTests.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicRemoteExecutorTests.cs index f57f4aef8ec670..974ba2183df060 100644 --- a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicRemoteExecutorTests.cs +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicRemoteExecutorTests.cs @@ -23,11 +23,6 @@ public MsQuicRemoteExecutorTests() [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] public void SslKeyLogFile_IsCreatedAndFilled() { - if (PlatformDetection.IsReleaseLibrary(typeof(QuicConnection).Assembly)) - { - throw new SkipTestException("Retrieving SSL secrets is not supported in Release mode."); - } - var psi = new ProcessStartInfo(); var tempFile = Path.GetTempFileName(); psi.Environment.Add("SSLKEYLOGFILE", tempFile); diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamRemoteExecutorTests.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamRemoteExecutorTests.cs index d162a54bf92172..d3dd83e1d2f286 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamRemoteExecutorTests.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamRemoteExecutorTests.cs @@ -22,14 +22,9 @@ public SslStreamRemoteExecutorTests() { } [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] - [ActiveIssue("https://github.com/dotnet/runtime/issues/94843", ~TestPlatforms.Linux)] + [PlatformSpecific(TestPlatforms.Linux)] // SSLKEYLOGFILE is only supported on Linux for SslStream public void SslKeyLogFile_IsCreatedAndFilled() { - if (PlatformDetection.IsReleaseLibrary(typeof(SslStream).Assembly)) - { - throw new SkipTestException("Retrieving SSL secrets is not supported in Release mode."); - } - var psi = new ProcessStartInfo(); var tempFile = Path.GetTempFileName(); psi.Environment.Add("SSLKEYLOGFILE", tempFile); @@ -61,4 +56,4 @@ await TestConfiguration.WhenAllOrAnyFailedWithTimeout( Assert.True(File.ReadAllText(tempFile).Length > 0); } } -} \ No newline at end of file +} From 5e87384231feb43a1d5a23d397fd7f26ca9497b2 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Fri, 5 Apr 2024 11:22:30 +0200 Subject: [PATCH 2/8] Unify SSLKEYLOGFILE logging, gate behind AppContextSwitch --- .../Interop.OpenSsl.cs | 15 +-- .../src/System/Net/Security/SslKeyLogger.cs | 121 ++++++++++++++++++ .../src/System.Net.Quic.csproj | 2 + .../Net/Quic/Internal/MsQuicTlsSecret.cs | 96 ++++++++------ .../MsQuicRemoteExecutorTests.cs | 28 +++- .../src/System.Net.Security.csproj | 2 + .../SslStreamRemoteExecutorTests.cs | 26 +++- .../System.Net.Security.Unit.Tests.csproj | 2 + 8 files changed, 231 insertions(+), 61 deletions(-) create mode 100644 src/libraries/Common/src/System/Net/Security/SslKeyLogger.cs diff --git a/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs b/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs index a1312c531c40bc..8636bbe4884866 100644 --- a/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs +++ b/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs @@ -21,8 +21,6 @@ internal static partial class Interop { internal static partial class OpenSsl { - private static readonly string? s_keyLogFile = Environment.GetEnvironmentVariable("SSLKEYLOGFILE"); - private static readonly FileStream? s_fileStream = s_keyLogFile != null ? File.Open(s_keyLogFile, FileMode.Append, FileAccess.Write, FileShare.ReadWrite) : null; private const string TlsCacheSizeCtxName = "System.Net.Security.TlsCacheSize"; private const string TlsCacheSizeEnvironmentVariable = "DOTNET_SYSTEM_NET_SECURITY_TLSCACHESIZE"; private const SslProtocols FakeAlpnSslProtocol = (SslProtocols)1; // used to distinguish server sessions with ALPN @@ -207,7 +205,7 @@ internal static unsafe SafeSslContextHandle AllocateSslContext(SslAuthentication Ssl.SslCtxSetDefaultOcspCallback(sslCtx); } } - if (s_fileStream != null) + if (SslKeyLogger.IsEnabled) { Ssl.SslCtxSetKeylogCallback(sslCtx, &KeyLogCallback); } @@ -756,17 +754,8 @@ private static unsafe void RemoveSessionCallback(IntPtr ctx, IntPtr session) [UnmanagedCallersOnly] private static unsafe void KeyLogCallback(IntPtr ssl, char* line) { - Debug.Assert(s_fileStream != null); ReadOnlySpan data = MemoryMarshal.CreateReadOnlySpanFromNullTerminated((byte*)line); - if (data.Length > 0) - { - lock (s_fileStream) - { - s_fileStream.Write(data); - s_fileStream.WriteByte((byte)'\n'); - s_fileStream.Flush(); - } - } + SslKeyLogger.WriteLineRaw(data); } private static int BioRead(SafeBioHandle bio, Span buffer, int count) diff --git a/src/libraries/Common/src/System/Net/Security/SslKeyLogger.cs b/src/libraries/Common/src/System/Net/Security/SslKeyLogger.cs new file mode 100644 index 00000000000000..5d3ca2c9b6ebba --- /dev/null +++ b/src/libraries/Common/src/System/Net/Security/SslKeyLogger.cs @@ -0,0 +1,121 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Diagnostics; +using System.IO; +using System.Net; + +internal static class SslKeyLogger +{ + private static readonly string? s_keyLogFile = Environment.GetEnvironmentVariable("SSLKEYLOGFILE"); + private static readonly FileStream? s_fileStream; + +#pragma warning disable CA1810 // Initialize all static fields when declared and remove cctor + static SslKeyLogger() + { + s_fileStream = null; + + try + { + bool isEnabled = AppContext.TryGetSwitch("System.Net.Security.EnableSslKeyLogging", out bool enabled) && enabled; + + if (isEnabled && s_keyLogFile != null) + { + s_fileStream = File.Open(s_keyLogFile, FileMode.Append, FileAccess.Write, FileShare.ReadWrite); + } + } + catch (Exception ex) + { + if (NetEventSource.Log.IsEnabled()) + { + NetEventSource.Error(null, $"Failed to open SSL key log file '{s_keyLogFile}': {ex}"); + } + } + } +#pragma warning restore CA1810 + + public static bool IsEnabled => s_fileStream != null; + + public static void WriteLineRaw(ReadOnlySpan data) + { + Debug.Assert(s_fileStream != null); + if (s_fileStream == null) + { + return; + } + + if (data.Length > 0) + { + lock (s_fileStream) + { + s_fileStream.Write(data); + s_fileStream.WriteByte((byte)'\n'); + s_fileStream.Flush(); + } + } + } + + public static void WriteSecrets( + ReadOnlySpan clientRandom, + ReadOnlySpan clientHandshakeTrafficSecret, + ReadOnlySpan serverHandshakeTrafficSecret, + ReadOnlySpan clientTrafficSecret0, + ReadOnlySpan serverTrafficSecret0, + ReadOnlySpan clientEarlyTrafficSecret) + { + Debug.Assert(s_fileStream != null); + Debug.Assert(!clientRandom.IsEmpty); + + if (s_fileStream == null || clientRandom.IsEmpty) + { + return; + } + + Span clientRandomUtf8 = clientRandom.Length <= 1024 ? stackalloc byte[clientRandom.Length * 2] : new byte[clientRandom.Length * 2]; + HexEncode(clientRandom, clientRandomUtf8); + + lock (s_fileStream) + { + WriteSecretCore("CLIENT_HANDSHAKE_TRAFFIC_SECRET"u8, clientRandomUtf8, clientHandshakeTrafficSecret); + WriteSecretCore("SERVER_HANDSHAKE_TRAFFIC_SECRET"u8, clientRandomUtf8, serverHandshakeTrafficSecret); + WriteSecretCore("CLIENT_TRAFFIC_SECRET_0"u8, clientRandomUtf8, clientTrafficSecret0); + WriteSecretCore("SERVER_TRAFFIC_SECRET_0"u8, clientRandomUtf8, serverTrafficSecret0); + WriteSecretCore("CLIENT_EARLY_TRAFFIC_SECRET"u8, clientRandomUtf8, clientEarlyTrafficSecret); + + s_fileStream.Flush(); + } + } + + private static void WriteSecretCore(ReadOnlySpan labelUtf8, ReadOnlySpan clientRandomUtf8, ReadOnlySpan secret) + { + Debug.Assert(s_fileStream != null); + if (secret.Length == 0) + { + return; + } + + int totalLength = labelUtf8.Length + 1 + clientRandomUtf8.Length + 1 + 2 * secret.Length + 1; + Span line = totalLength <= 1024 ? stackalloc byte[totalLength] : new byte[totalLength]; + + labelUtf8.CopyTo(line); + line[labelUtf8.Length] = (byte)' '; + + clientRandomUtf8.CopyTo(line.Slice(labelUtf8.Length + 1)); + line[labelUtf8.Length + 1 + clientRandomUtf8.Length] = (byte)' '; + + HexEncode(secret, line.Slice(labelUtf8.Length + 1 + clientRandomUtf8.Length + 1)); + line[^1] = (byte)'\n'; + + s_fileStream.Write(line); + } + + private static void HexEncode(ReadOnlySpan source, Span destination) + { + for (int i = 0; i < source.Length; i++) + { + HexConverter.ToBytesBuffer(source[i], destination.Slice(i * 2)); + } + } + +} diff --git a/src/libraries/System.Net.Quic/src/System.Net.Quic.csproj b/src/libraries/System.Net.Quic/src/System.Net.Quic.csproj index aafca7f3e93b2a..bc091740b47473 100644 --- a/src/libraries/System.Net.Quic/src/System.Net.Quic.csproj +++ b/src/libraries/System.Net.Quic/src/System.Net.Quic.csproj @@ -38,6 +38,8 @@ + diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicTlsSecret.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicTlsSecret.cs index 26f69c1653aa83..44f57b8b86bcfe 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicTlsSecret.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicTlsSecret.cs @@ -1,7 +1,6 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -#if DEBUG using System.Diagnostics; using System.IO; using System.Runtime.InteropServices; @@ -13,14 +12,11 @@ namespace System.Net.Quic; internal sealed class MsQuicTlsSecret : IDisposable { - private static readonly string? s_keyLogFile = Environment.GetEnvironmentVariable("SSLKEYLOGFILE"); - private static readonly FileStream? s_fileStream = s_keyLogFile != null ? File.Open(s_keyLogFile, FileMode.Append, FileAccess.Write, FileShare.ReadWrite) : null; - private unsafe QUIC_TLS_SECRETS* _tlsSecrets; public static unsafe MsQuicTlsSecret? Create(MsQuicContextSafeHandle handle) { - if (s_fileStream is null) + if (!SslKeyLogger.IsEnabled) { return null; } @@ -55,40 +51,69 @@ private unsafe MsQuicTlsSecret(QUIC_TLS_SECRETS* tlsSecrets) public unsafe void WriteSecret() { - Debug.Assert(_tlsSecrets is not null); - Debug.Assert(s_fileStream is not null); + ReadOnlySpan clientRandom = _tlsSecrets->IsSet.ClientRandom != 0 + ? new ReadOnlySpan(_tlsSecrets->ClientRandom, 32) + : ReadOnlySpan.Empty; + + Span clientHandshakeTrafficSecret = _tlsSecrets->IsSet.ClientHandshakeTrafficSecret != 0 + ? new Span(_tlsSecrets->ClientHandshakeTrafficSecret, _tlsSecrets->SecretLength) + : Span.Empty; + + Span serverHandshakeTrafficSecret = _tlsSecrets->IsSet.ServerHandshakeTrafficSecret != 0 + ? new Span(_tlsSecrets->ServerHandshakeTrafficSecret, _tlsSecrets->SecretLength) + : Span.Empty; + + Span clientTrafficSecret0 = _tlsSecrets->IsSet.ClientTrafficSecret0 != 0 + ? new Span(_tlsSecrets->ClientTrafficSecret0, _tlsSecrets->SecretLength) + : Span.Empty; - lock (s_fileStream) + Span serverTrafficSecret0 = _tlsSecrets->IsSet.ServerTrafficSecret0 != 0 + ? new Span(_tlsSecrets->ServerTrafficSecret0, _tlsSecrets->SecretLength) + : Span.Empty; + + Span clientEarlyTrafficSecret = _tlsSecrets->IsSet.ClientEarlyTrafficSecret != 0 + ? new Span(_tlsSecrets->ClientEarlyTrafficSecret, _tlsSecrets->SecretLength) + : Span.Empty; + + SslKeyLogger.WriteSecrets( + clientRandom, + clientHandshakeTrafficSecret, + serverHandshakeTrafficSecret, + clientTrafficSecret0, + serverTrafficSecret0, + clientEarlyTrafficSecret); + + // clear secrets already logged, so they are not logged again on next call, + // keep ClientRandom as it is used for all secrets (and is not a secret itself) + if (!clientHandshakeTrafficSecret.IsEmpty) { - string clientRandom = string.Empty; - if (_tlsSecrets->IsSet.ClientRandom != 0) - { - clientRandom = Convert.ToHexString(new ReadOnlySpan(_tlsSecrets->ClientRandom, 32)); - } - if (_tlsSecrets->IsSet.ClientHandshakeTrafficSecret != 0) - { - s_fileStream.Write(Encoding.ASCII.GetBytes($"CLIENT_HANDSHAKE_TRAFFIC_SECRET {clientRandom} {Convert.ToHexString(new ReadOnlySpan(_tlsSecrets->ClientHandshakeTrafficSecret, _tlsSecrets->SecretLength))}\n")); - } - if (_tlsSecrets->IsSet.ServerHandshakeTrafficSecret != 0) - { - s_fileStream.Write(Encoding.ASCII.GetBytes($"SERVER_HANDSHAKE_TRAFFIC_SECRET {clientRandom} {Convert.ToHexString(new ReadOnlySpan(_tlsSecrets->ServerHandshakeTrafficSecret, _tlsSecrets->SecretLength))}\n")); - } - if (_tlsSecrets->IsSet.ClientTrafficSecret0 != 0) - { - s_fileStream.Write(Encoding.ASCII.GetBytes($"CLIENT_TRAFFIC_SECRET_0 {clientRandom} {Convert.ToHexString(new ReadOnlySpan(_tlsSecrets->ClientTrafficSecret0, _tlsSecrets->SecretLength))}\n")); - } - if (_tlsSecrets->IsSet.ServerTrafficSecret0 != 0) - { - s_fileStream.Write(Encoding.ASCII.GetBytes($"SERVER_TRAFFIC_SECRET_0 {clientRandom} {Convert.ToHexString(new ReadOnlySpan(_tlsSecrets->ServerTrafficSecret0, _tlsSecrets->SecretLength))}\n")); - } - if (_tlsSecrets->IsSet.ClientEarlyTrafficSecret != 0) - { - s_fileStream.Write(Encoding.ASCII.GetBytes($"CLIENT_EARLY_TRAFFIC_SECRET {clientRandom} {Convert.ToHexString(new ReadOnlySpan(_tlsSecrets->ClientEarlyTrafficSecret, _tlsSecrets->SecretLength))}\n")); - } - s_fileStream.Flush(); + clientHandshakeTrafficSecret.Clear(); + _tlsSecrets->IsSet.ClientHandshakeTrafficSecret = 0; } - NativeMemory.Clear(_tlsSecrets, (nuint)sizeof(QUIC_TLS_SECRETS)); + if (!serverHandshakeTrafficSecret.IsEmpty) + { + serverHandshakeTrafficSecret.Clear(); + _tlsSecrets->IsSet.ServerHandshakeTrafficSecret = 0; + } + + if (!clientTrafficSecret0.IsEmpty) + { + clientTrafficSecret0.Clear(); + _tlsSecrets->IsSet.ClientTrafficSecret0 = 0; + } + + if (!serverTrafficSecret0.IsEmpty) + { + serverTrafficSecret0.Clear(); + _tlsSecrets->IsSet.ServerTrafficSecret0 = 0; + } + + if (!clientEarlyTrafficSecret.IsEmpty) + { + clientEarlyTrafficSecret.Clear(); + _tlsSecrets->IsSet.ClientEarlyTrafficSecret = 0; + } } public unsafe void Dispose() @@ -110,4 +135,3 @@ public unsafe void Dispose() } } } -#endif diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicRemoteExecutorTests.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicRemoteExecutorTests.cs index 974ba2183df060..493d0d74bc9028 100644 --- a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicRemoteExecutorTests.cs +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicRemoteExecutorTests.cs @@ -20,22 +20,38 @@ public class MsQuicRemoteExecutorTests : QuicTestBase public MsQuicRemoteExecutorTests() : base(null!) { } - [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] - public void SslKeyLogFile_IsCreatedAndFilled() + [ConditionalTheory(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] + [InlineData(true)] + [InlineData(false)] + public void SslKeyLogFile_IsCreatedAndFilled(bool enabledBySwitch) { var psi = new ProcessStartInfo(); var tempFile = Path.GetTempFileName(); psi.Environment.Add("SSLKEYLOGFILE", tempFile); - RemoteExecutor.Invoke(async () => + RemoteExecutor.Invoke(async (enabledBySwitch) => { + if (bool.Parse(enabledBySwitch)) + { + AppContext.SetSwitch("System.Net.Security.EnableSslKeyLogging", true); + } + (QuicConnection clientConnection, QuicConnection serverConnection) = await CreateConnectedQuicConnection(); await clientConnection.DisposeAsync(); await serverConnection.DisposeAsync(); - }, new RemoteInvokeOptions { StartInfo = psi }).Dispose(); + } + , enabledBySwitch.ToString(), new RemoteInvokeOptions { StartInfo = psi }).Dispose(); - Assert.True(File.Exists(tempFile)); - Assert.True(File.ReadAllText(tempFile).Length > 0); + if (enabledBySwitch) + { + Assert.True(File.Exists(tempFile)); + var text = File.ReadAllText(tempFile); + Assert.True(text.Length > 0); + } + else + { + Assert.True(!File.Exists(tempFile) || File.ReadAllText(tempFile).Length == 0); + } } } } diff --git a/src/libraries/System.Net.Security/src/System.Net.Security.csproj b/src/libraries/System.Net.Security/src/System.Net.Security.csproj index 9a7fd09fd88b13..e6650f80670e17 100644 --- a/src/libraries/System.Net.Security/src/System.Net.Security.csproj +++ b/src/libraries/System.Net.Security/src/System.Net.Security.csproj @@ -387,6 +387,8 @@ Link="Common\Microsoft\Win32\SafeHandles\SafeHandleCache.cs" /> + diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamRemoteExecutorTests.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamRemoteExecutorTests.cs index d3dd83e1d2f286..d3c87ad8b9c95f 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamRemoteExecutorTests.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamRemoteExecutorTests.cs @@ -21,16 +21,23 @@ public class SslStreamRemoteExecutorTests public SslStreamRemoteExecutorTests() { } - [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] + [ConditionalTheory(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] [PlatformSpecific(TestPlatforms.Linux)] // SSLKEYLOGFILE is only supported on Linux for SslStream - public void SslKeyLogFile_IsCreatedAndFilled() + [InlineData(true)] + [InlineData(false)] + public void SslKeyLogFile_IsCreatedAndFilled(bool enabledBySwitch) { var psi = new ProcessStartInfo(); var tempFile = Path.GetTempFileName(); psi.Environment.Add("SSLKEYLOGFILE", tempFile); - RemoteExecutor.Invoke(async () => + RemoteExecutor.Invoke(async (enabledBySwitch) => { + if (bool.Parse(enabledBySwitch)) + { + AppContext.SetSwitch("System.Net.Security.EnableSslKeyLogging", true); + } + (Stream clientStream, Stream serverStream) = TestHelper.GetConnectedStreams(); using (clientStream) using (serverStream) @@ -50,10 +57,17 @@ await TestConfiguration.WhenAllOrAnyFailedWithTimeout( await TestHelper.PingPong(client, server); } - }, new RemoteInvokeOptions { StartInfo = psi }).Dispose(); + }, enabledBySwitch.ToString(), new RemoteInvokeOptions { StartInfo = psi }).Dispose(); - Assert.True(File.Exists(tempFile)); - Assert.True(File.ReadAllText(tempFile).Length > 0); + if (enabledBySwitch) + { + Assert.True(File.Exists(tempFile)); + Assert.True(File.ReadAllText(tempFile).Length > 0); + } + else + { + Assert.True(!File.Exists(tempFile) || File.ReadAllText(tempFile).Length == 0); + } } } } diff --git a/src/libraries/System.Net.Security/tests/UnitTests/System.Net.Security.Unit.Tests.csproj b/src/libraries/System.Net.Security/tests/UnitTests/System.Net.Security.Unit.Tests.csproj index e3e81ecb61a3d4..5602fc0cab3562 100644 --- a/src/libraries/System.Net.Security/tests/UnitTests/System.Net.Security.Unit.Tests.csproj +++ b/src/libraries/System.Net.Security/tests/UnitTests/System.Net.Security.Unit.Tests.csproj @@ -121,6 +121,8 @@ Link="Common\Microsoft\Win32\SafeHandles\SafeHandleCache.cs" /> + From 6b70be8c9803da939a398e61f721215fca295e05 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Fri, 5 Apr 2024 11:36:04 +0200 Subject: [PATCH 3/8] Add more points where QUIC secrets are logged --- .../Common/src/System/Net/Security/SslKeyLogger.cs | 13 ++++++++++--- .../src/System/Net/Quic/QuicConnection.cs | 7 +++++++ 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/src/libraries/Common/src/System/Net/Security/SslKeyLogger.cs b/src/libraries/Common/src/System/Net/Security/SslKeyLogger.cs index 5d3ca2c9b6ebba..f638dea1e62e35 100644 --- a/src/libraries/Common/src/System/Net/Security/SslKeyLogger.cs +++ b/src/libraries/Common/src/System/Net/Security/SslKeyLogger.cs @@ -67,7 +67,15 @@ public static void WriteSecrets( Debug.Assert(s_fileStream != null); Debug.Assert(!clientRandom.IsEmpty); - if (s_fileStream == null || clientRandom.IsEmpty) + if (s_fileStream == null || + clientRandom.IsEmpty || + + // return early if there is nothing to log + (clientHandshakeTrafficSecret.IsEmpty && + serverHandshakeTrafficSecret.IsEmpty && + clientTrafficSecret0.IsEmpty && + serverTrafficSecret0.IsEmpty && + clientEarlyTrafficSecret.IsEmpty)) { return; } @@ -89,7 +97,6 @@ public static void WriteSecrets( private static void WriteSecretCore(ReadOnlySpan labelUtf8, ReadOnlySpan clientRandomUtf8, ReadOnlySpan secret) { - Debug.Assert(s_fileStream != null); if (secret.Length == 0) { return; @@ -107,7 +114,7 @@ private static void WriteSecretCore(ReadOnlySpan labelUtf8, ReadOnlySpan source, Span destination) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs index 876ea3299e596f..315352d2797bb4 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs @@ -504,6 +504,7 @@ private unsafe int HandleEventConnected(ref CONNECTED_DATA data) QuicAddr localAddress = MsQuicHelpers.GetMsQuicParameter(_handle, QUIC_PARAM_CONN_LOCAL_ADDRESS); _localEndPoint = MsQuicHelpers.QuicAddrToIPEndPoint(&localAddress); + // Final (1-RTT) secrets have been derived, log them if desired to allow decrypting application traffic. _tlsSecret?.WriteSecret(); if (NetEventSource.Log.IsEnabled()) @@ -527,6 +528,9 @@ private unsafe int HandleEventShutdownInitiatedByPeer(ref SHUTDOWN_INITIATED_BY_ } private unsafe int HandleEventShutdownComplete() { + // make sure we log at least some secrets in case of shutdown before handshake completes. + _tlsSecret?.WriteSecret(); + Exception exception = ExceptionDispatchInfo.SetCurrentStackTrace(_disposed == 1 ? new ObjectDisposedException(GetType().FullName) : ThrowHelper.GetOperationAbortedException()); _acceptQueue.Writer.TryComplete(exception); _connectedTcs.TrySetException(exception); @@ -570,6 +574,9 @@ private unsafe int HandleEventPeerCertificateReceived(ref PEER_CERTIFICATE_RECEI // worker threads. // + // Handshake keys should be available by now, log them now if desired. + _tlsSecret?.WriteSecret(); + var task = _sslConnectionOptions.StartAsyncCertificateValidation((IntPtr)data.Certificate, (IntPtr)data.Chain); if (task.IsCompletedSuccessfully) { From 06c9429a4e63de50ed98d8018df4ff98ffdc4ae9 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Fri, 5 Apr 2024 11:41:41 +0200 Subject: [PATCH 4/8] Enable in Debug builds without appctx switch --- src/libraries/Common/src/System/Net/Security/SslKeyLogger.cs | 4 ++++ .../tests/FunctionalTests/MsQuicRemoteExecutorTests.cs | 5 +++++ .../tests/FunctionalTests/SslStreamRemoteExecutorTests.cs | 5 +++++ 3 files changed, 14 insertions(+) diff --git a/src/libraries/Common/src/System/Net/Security/SslKeyLogger.cs b/src/libraries/Common/src/System/Net/Security/SslKeyLogger.cs index f638dea1e62e35..e11f523e0c8312 100644 --- a/src/libraries/Common/src/System/Net/Security/SslKeyLogger.cs +++ b/src/libraries/Common/src/System/Net/Security/SslKeyLogger.cs @@ -18,7 +18,11 @@ static SslKeyLogger() try { +#if DEBUG + bool isEnabled = true; +#else bool isEnabled = AppContext.TryGetSwitch("System.Net.Security.EnableSslKeyLogging", out bool enabled) && enabled; +#endif if (isEnabled && s_keyLogFile != null) { diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicRemoteExecutorTests.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicRemoteExecutorTests.cs index 493d0d74bc9028..8f18626511ae7f 100644 --- a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicRemoteExecutorTests.cs +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicRemoteExecutorTests.cs @@ -25,6 +25,11 @@ public MsQuicRemoteExecutorTests() [InlineData(false)] public void SslKeyLogFile_IsCreatedAndFilled(bool enabledBySwitch) { + if (PlatformDetection.IsDebugLibrary(typeof(QuicConnection).Assembly) && !enabledBySwitch) + { + throw new SkipTestException("AppCtxSwitch is not necessary for SSLKEYLOGFILE in Debug."); + } + var psi = new ProcessStartInfo(); var tempFile = Path.GetTempFileName(); psi.Environment.Add("SSLKEYLOGFILE", tempFile); diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamRemoteExecutorTests.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamRemoteExecutorTests.cs index d3c87ad8b9c95f..fe5022b2312a99 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamRemoteExecutorTests.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamRemoteExecutorTests.cs @@ -27,6 +27,11 @@ public SslStreamRemoteExecutorTests() [InlineData(false)] public void SslKeyLogFile_IsCreatedAndFilled(bool enabledBySwitch) { + if (PlatformDetection.IsDebugLibrary(typeof(SslStream).Assembly) && !enabledBySwitch) + { + throw new SkipTestException("AppCtxSwitch is not necessary for SSLKEYLOGFILE in Debug."); + } + var psi = new ProcessStartInfo(); var tempFile = Path.GetTempFileName(); psi.Environment.Add("SSLKEYLOGFILE", tempFile); From ad972102b7dd2c5a583637cb92e436b86ef8820c Mon Sep 17 00:00:00 2001 From: Radek Zikmund <32671551+rzikm@users.noreply.github.com> Date: Fri, 5 Apr 2024 14:10:21 +0200 Subject: [PATCH 5/8] Update src/libraries/System.Net.Quic/src/System.Net.Quic.csproj MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Marie Píchová <11718369+ManickaP@users.noreply.github.com> --- src/libraries/System.Net.Quic/src/System.Net.Quic.csproj | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/libraries/System.Net.Quic/src/System.Net.Quic.csproj b/src/libraries/System.Net.Quic/src/System.Net.Quic.csproj index bc091740b47473..8245e3304ad69c 100644 --- a/src/libraries/System.Net.Quic/src/System.Net.Quic.csproj +++ b/src/libraries/System.Net.Quic/src/System.Net.Quic.csproj @@ -38,8 +38,7 @@ - + From 6806a61bd1de6c249d3ab38c245670ea77248780 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Mon, 8 Apr 2024 10:21:43 +0200 Subject: [PATCH 6/8] Code review feedback --- .../Common/src/System/Net/Security/SslKeyLogger.cs | 2 ++ .../tests/FunctionalTests/MsQuicRemoteExecutorTests.cs | 6 ++---- .../tests/FunctionalTests/SslStreamRemoteExecutorTests.cs | 3 +-- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/libraries/Common/src/System/Net/Security/SslKeyLogger.cs b/src/libraries/Common/src/System/Net/Security/SslKeyLogger.cs index e11f523e0c8312..85f8566597f60a 100644 --- a/src/libraries/Common/src/System/Net/Security/SslKeyLogger.cs +++ b/src/libraries/Common/src/System/Net/Security/SslKeyLogger.cs @@ -106,6 +106,8 @@ private static void WriteSecretCore(ReadOnlySpan labelUtf8, ReadOnlySpan line = totalLength <= 1024 ? stackalloc byte[totalLength] : new byte[totalLength]; diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicRemoteExecutorTests.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicRemoteExecutorTests.cs index 8f18626511ae7f..922408248b3bff 100644 --- a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicRemoteExecutorTests.cs +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicRemoteExecutorTests.cs @@ -49,13 +49,11 @@ public void SslKeyLogFile_IsCreatedAndFilled(bool enabledBySwitch) if (enabledBySwitch) { - Assert.True(File.Exists(tempFile)); - var text = File.ReadAllText(tempFile); - Assert.True(text.Length > 0); + Assert.True(File.ReadAllText(tempFile).Length > 0); } else { - Assert.True(!File.Exists(tempFile) || File.ReadAllText(tempFile).Length == 0); + Assert.True(File.ReadAllText(tempFile).Length == 0); } } } diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamRemoteExecutorTests.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamRemoteExecutorTests.cs index fe5022b2312a99..a4c7e975d92b21 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamRemoteExecutorTests.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamRemoteExecutorTests.cs @@ -66,12 +66,11 @@ await TestConfiguration.WhenAllOrAnyFailedWithTimeout( if (enabledBySwitch) { - Assert.True(File.Exists(tempFile)); Assert.True(File.ReadAllText(tempFile).Length > 0); } else { - Assert.True(!File.Exists(tempFile) || File.ReadAllText(tempFile).Length == 0); + Assert.True(File.ReadAllText(tempFile).Length == 0); } } } From 0256cf1a44eca88758c06c039a22f683c5f679ac Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Mon, 8 Apr 2024 15:41:19 +0200 Subject: [PATCH 7/8] More code review changes --- .../tests/FunctionalTests/MsQuicRemoteExecutorTests.cs | 4 +++- .../tests/FunctionalTests/SslStreamRemoteExecutorTests.cs | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicRemoteExecutorTests.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicRemoteExecutorTests.cs index 922408248b3bff..618fca3cbeae76 100644 --- a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicRemoteExecutorTests.cs +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicRemoteExecutorTests.cs @@ -27,7 +27,9 @@ public void SslKeyLogFile_IsCreatedAndFilled(bool enabledBySwitch) { if (PlatformDetection.IsDebugLibrary(typeof(QuicConnection).Assembly) && !enabledBySwitch) { - throw new SkipTestException("AppCtxSwitch is not necessary for SSLKEYLOGFILE in Debug."); + // AppCtxSwitch is not checked for SSLKEYLOGFILE in Debug builds, the same code path + // will be tested by the enabledBySwitch = true case. Skip it here. + return; } var psi = new ProcessStartInfo(); diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamRemoteExecutorTests.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamRemoteExecutorTests.cs index a4c7e975d92b21..a4b34b95f4af6b 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamRemoteExecutorTests.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamRemoteExecutorTests.cs @@ -29,7 +29,9 @@ public void SslKeyLogFile_IsCreatedAndFilled(bool enabledBySwitch) { if (PlatformDetection.IsDebugLibrary(typeof(SslStream).Assembly) && !enabledBySwitch) { - throw new SkipTestException("AppCtxSwitch is not necessary for SSLKEYLOGFILE in Debug."); + // AppCtxSwitch is not checked for SSLKEYLOGFILE in Debug builds, the same code path + // will be tested by the enabledBySwitch = true case. Skip it here. + return; } var psi = new ProcessStartInfo(); From 97822daf27a15e8d3e21006472a8af89da3d3bad Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Tue, 9 Apr 2024 20:17:17 +0200 Subject: [PATCH 8/8] iAdjust appctx switch name --- src/libraries/Common/src/System/Net/Security/SslKeyLogger.cs | 2 +- .../tests/FunctionalTests/MsQuicRemoteExecutorTests.cs | 2 +- .../tests/FunctionalTests/SslStreamRemoteExecutorTests.cs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libraries/Common/src/System/Net/Security/SslKeyLogger.cs b/src/libraries/Common/src/System/Net/Security/SslKeyLogger.cs index 85f8566597f60a..e6e9d9cd039297 100644 --- a/src/libraries/Common/src/System/Net/Security/SslKeyLogger.cs +++ b/src/libraries/Common/src/System/Net/Security/SslKeyLogger.cs @@ -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; #endif if (isEnabled && s_keyLogFile != null) diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicRemoteExecutorTests.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicRemoteExecutorTests.cs index 618fca3cbeae76..d2a40a1f35af53 100644 --- a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicRemoteExecutorTests.cs +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicRemoteExecutorTests.cs @@ -40,7 +40,7 @@ public void SslKeyLogFile_IsCreatedAndFilled(bool enabledBySwitch) { if (bool.Parse(enabledBySwitch)) { - AppContext.SetSwitch("System.Net.Security.EnableSslKeyLogging", true); + AppContext.SetSwitch("System.Net.EnableSslKeyLogging", true); } (QuicConnection clientConnection, QuicConnection serverConnection) = await CreateConnectedQuicConnection(); diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamRemoteExecutorTests.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamRemoteExecutorTests.cs index a4b34b95f4af6b..82cd3114eb4772 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamRemoteExecutorTests.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamRemoteExecutorTests.cs @@ -42,7 +42,7 @@ public void SslKeyLogFile_IsCreatedAndFilled(bool enabledBySwitch) { if (bool.Parse(enabledBySwitch)) { - AppContext.SetSwitch("System.Net.Security.EnableSslKeyLogging", true); + AppContext.SetSwitch("System.Net.EnableSslKeyLogging", true); } (Stream clientStream, Stream serverStream) = TestHelper.GetConnectedStreams();