add support of TLS 1.3 on macOS POC #104835

wfurt · 2024-07-12T21:25:08Z

This contributes to #1979 that is long due. The API we currently use is deprecated since 10.5 Catalina and can possibly go at any time. Also with recent security push there may be more TLS 1.3 only servers and there really no good workaround for macOS and iOS so those platforms will simply fail to connect.

There are conceptually two challenges.

The new API does not provide capability to work on top of buffers like SslStream expects. To over come it we use Framer approach pretending we are implementing custom protocol. Something @filipnavara demonstrated here: https://gist.github.com/filipnavara/d5fb55bdb5edcceb1981f73078b855c4

The second is that there is no synchronize API. We basically call void function and we provide lambda that would be called once in the future. While that may be generally handy and similar to our 'async` it creates significant complication and platform difference.

To do that both handshake and decrypt have new Task that deliver events. I tried to structure it in a way that it has no impact on other platform. There can perhaps be improvements here. Certainly the "decrypt in place is a lieand we can possibly avoid some copies and memory use by refactoring. Original naive implementation was hitting assets for synchronous API - something I fixed by tracking sync vs async now and using synchronousWhenAny` when needed.

This PR is trying to close the gap by addressing both. It is still in rough shape but I would like to solicit early feedback before spending too much time polishing. I can do basic operations and pass most of the test locally:

=== TEST EXECUTION SUMMARY ===
     System.Net.Security.Tests  Total: 577, Errors: 0, Failed: 9, Skipped: 16, Time: 405.069s

Since the implementation is not complete I keep old one around as well and I use new one only when seems appropriate.
I also envision some possible compat switch so user can opt-in or opt-out as needed. The focus is on client for developers talking to server they cannot control. For simple HTTP(s) it should just work.

From failing tests, the new API is more picky about non ASCIS in SNI name. Something we saw on Android, but it should not matter for HTTP as we puny-code IDN.

Some tests (particularly around cipher suites) are confused as we cannot handshake TLS 1.3 in tests but the client side is capable of doing it.

And there are still some loose ends around closing, disposing and cleanup.

it would be great to get feedback, particularly from
@jkotas for inter
@stephentoub for async
@filipnavara for objective-C and macOS API use
@rzikm for integration with SslStream

and @dotnet/ncl for all the above. Note that the Network Framework can also do Quic so this may be option for use in the distant future.

dotnet-policy-service · 2024-07-12T21:25:34Z

Tagging subscribers to this area: @dotnet/ncl, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

liveans · 2024-07-15T11:56:33Z

src/native/libs/System.Security.Cryptography.Native.Apple/pal_ssl.m

@@ -0,0 +1,429 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+


This file looks like C with some AppleClang extensions, rather than objective C

This is Objective-C e.g. .m vs .c file type.

rzikm

Looks interesting so far, but it feels like a substantial change which we are unlikely to get into 9.0

rzikm · 2024-07-15T12:19:28Z

src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.Ssl.cs


+        [LibraryImport(Interop.Libraries.AppleCryptoNative, EntryPoint = "AppleCryptoNative_NwCreateContext")]
+        internal static partial System.Net.SafeSslHandle NwCreateContext(int isServer);
+
+        [LibraryImport(Interop.Libraries.AppleCryptoNative, EntryPoint = "AppleCryptoNative_NwInit")]
+        internal static unsafe partial int NwInit(delegate* unmanaged<IntPtr, PAL_NwStatusUpdates, IntPtr, IntPtr, int> statusCallback, delegate* unmanaged<IntPtr, byte*, void**, int> readCallback, delegate* unmanaged<IntPtr, byte*, void**, int> writeCallback);


I assume Nw* and Ssl* functions can't be used interchangeably, would it make sense to move it to its own class like Interop.NetworkFramework?

This would be helpful, especially for the possible future implementations/experiments. (QUIC API is there and can be used with couple of more interop additions to this. (there are some bugs))

rzikm · 2024-07-15T12:39:14Z

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.IO.cs

@@ -251,11 +253,13 @@ private async Task RenegotiateAsync<TIOAdapter>(CancellationToken cancellationTo
        }

        // reAuthenticationData is only used on Windows in case of renegotiation.
-        private async Task ForceAuthenticationAsync<TIOAdapter>(bool receiveFirst, byte[]? reAuthenticationData, CancellationToken cancellationToken)
+        private async Task ForceAuthenticationAsync<TIOAdapter>(bool receiveFirst, byte[]? reAuthenticationData, CancellationToken cancellationToken, bool isAsync = true)


The extra isAsync parameter is a bit weird, since the information should already be apparent from the TIOAdapter. Is this expected to stay? So far the usages of this inside the function seem debug-only or WIP code.

rzikm · 2024-07-15T12:48:30Z

src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs

+                int toWrite = (int)length;
+                var inputBuffer = new ReadOnlySpan<byte>(data, toWrite);
+
+                if (context.UseNwFramework)


There are now lots of such branches all over the codebase, It might be a lot of work but reshuffling the code s.t code-paths which uses NetworkFramework is in it's own class (SafeDeleteNwSslContext maybe?) would help readability IMO.

rzikm · 2024-07-15T12:50:18Z

src/libraries/System.Net.Security/src/System/Net/SecurityStatusPal.cs

@@ -34,6 +34,7 @@ internal enum SecurityStatusPalErrorCode
        Renegotiate,
        TryAgain,
        HandshakeStarted,
+        ContinuePendig,


Suggested change

ContinuePendig,

ContinuePending,

wfurt · 2024-07-15T17:13:30Z

Looks interesting so far, but it feels like a substantial change which we are unlikely to get into 9.0

My goal would be to make it op-in feature like we did for managed NTLM. e.g. make minimal impact by default but provide something for users who would be stuck otherwise.

stephentoub · 2024-07-18T21:00:56Z

src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.Ssl.cs

+
+        internal static unsafe int NwSetTlsOptions(SafeSslHandle sslHandle, nint gcHandle, string targetName, List<SslApplicationProtocol>? applicationProtocols, SslProtocols minTlsVersion, SslProtocols maxTlsVersion)
+        {
+            int alpnLength = GetAlpnProtocolListSerializedLength(applicationProtocols);


nit: indentation

stephentoub · 2024-07-18T21:01:05Z

src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.Ssl.cs

+        [LibraryImport(Interop.Libraries.AppleCryptoNative, EntryPoint = "AppleCryptoNative_NwCancelConnection")]
+        internal static partial int NwCancelConnection(SafeSslHandle sslHandle);
+
+


Suggested change

stephentoub · 2024-07-19T21:25:32Z

src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.Ssl.cs

+                return;
+            }
+
+            Debug.Assert(GetAlpnProtocolListSerializedLength(applicationProtocols) == buffer.Length,


FWIW, you no longer need this message. The exact same message will now be injected for you,

stephentoub · 2024-07-19T21:37:22Z

src/libraries/Common/src/System/Net/ArrayBuffer.cs

@@ -53,6 +54,7 @@ public ArrayBuffer(byte[] buffer)

        public void Dispose()
        {
+        _disposed = true;


nit: indenting

stephentoub · 2024-07-19T21:37:31Z

src/libraries/Common/tests/StreamConformanceTests/System/IO/StreamConformanceTests.cs

@@ -2864,7 +2865,7 @@ public virtual async Task UseWrappedAfterClose_Success()
                readable.ReadByte();
            }
        }
-
+*/


stephentoub · 2024-07-19T21:52:17Z

src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs

@@ -355,12 +620,32 @@ internal int ReadPendingWrites(byte[] buf, int offset, int count)
            SslProtocols.Tls12
        };

-        private static void SetProtocols(SafeSslHandle sslContext, SslProtocols protocols)
+        private static readonly SslProtocols[] s_orderedSslProtocols13 = new SslProtocols[6]


Can this be:

private static ReadOnlySpan<SslProtocols> OrderedSslProtocols13 => [ #pragma warning disable 0618 SslProtocols.Ssl2, SslProtocols.Ssl3, #pragma warning restore 0618 #pragma warning disable SYSLIB0039 // TLS 1.0 and 1.1 are obsolete SslProtocols.Tls, SslProtocols.Tls11, #pragma warning restore SYSLIB0039 SslProtocols.Tls12, SslProtocols.Tls13 ];

?

stephentoub · 2024-07-19T21:55:07Z

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.IO.cs

+            // if we have background task eat any exceptions
+            _frameTask?.ContinueWith(t => {
+                     _ = t.Exception;
+                     _buffer.ReturnBuffer();


What prevents the buffer from being returned twice?

stephentoub · 2024-07-19T21:56:17Z

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.IO.cs

+                            // This is optimization to consume and report alters insteads of throwing IO excceoption as
+                            // the peer would typically close connection afterwards.
+                            // We don't want to thorw here, taht would be done later if needed
+                            Task.WaitAny(new Task[] { handshakeTask }, cancellationToken);


Why is this always synchronous regardless of whether isSync?

Also, the cancellation token here will cancel the waiting, but it won't affect handshakeTask... that's ok? Will handshakeTask be canceled separately in response to cancellationToken?

stephentoub · 2024-07-19T21:57:42Z

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.IO.cs

+                            if (isSync)
+                            {
+                                Task[] tasks = new Task[] { handshakeTask, _frameTask };
+                                int index = Task.WaitAny(tasks, cancellationToken);


Why is synchronous waiting being canceled with the token but asynchronous waiting isn't?

stephentoub · 2024-07-19T22:12:54Z

src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs

+                // We grab reference to prevent disposal while handshake is pending.
+                this.DangerousAddRef(ref add);
+                ObjectDisposedException.ThrowIf(_disposed, this);
+                Interop.AppleCrypto.NwStartHandshake(SslContext, GCHandle.ToIntPtr(gcHandle));


Seems like you might need some GC.KeepAlives in various places. This gcHandle is a weak handle, which means it's not keeping this alive, which means if nothing after the call to this instance method is guaranteed to be keeping the instance alive, it's possible it could be collected at a bad time around here.

dotnet-policy-service · 2024-08-18T23:24:53Z

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

dotnet-policy-service · 2024-09-18T20:19:17Z

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

wfurt added 9 commits July 9, 2024 10:12

snap

f5d116d

tests

c3be4f3

snap

86665f6

snap

7c3e67b

snap

02c1eb3

snap

11edad8

snap

fcc9879

snap

7361d26

snap

53b69f6

wfurt added area-System.Net.Security os-mac-os-x macOS aka OSX labels Jul 12, 2024

wfurt requested review from filipnavara, stephentoub, jkotas and rzikm July 12, 2024 21:25

wfurt self-assigned this Jul 12, 2024

Merge branch 'main' of https://github.com/dotnet/runtime into macNw

093d9bb

liveans reviewed Jul 15, 2024

View reviewed changes

rzikm reviewed Jul 15, 2024

View reviewed changes

wfurt added 2 commits July 15, 2024 14:35

update

83901ca

update

3ca7fbc

build-analysis bot mentioned this pull request Jul 16, 2024

Intermittent build failure in AfterSourceBuild: "Could not write state file" #76488

Open

wfurt added 2 commits July 15, 2024 18:11

build update

d07c7b9

build

ad8fadf

jkotas removed their request for review July 16, 2024 04:34

wfurt added 10 commits July 17, 2024 16:24

Merge branch 'main' of https://github.com/dotnet/runtime into macNw

95e0c33

Merge branch 'main' of https://github.com/dotnet/runtime into macNw

89c0444

build

7fec8ae

Merge branch 'main' of https://github.com/dotnet/runtime into macNw

b8b64d0

Merge branch 'macNw' of https://github.com/wfurt/runtime into macNw

9d5b512

aot

97f59d2

builder

b643236

Merge branch 'macNw' of https://github.com/wfurt/runtime into macNw

40f31e1

Merge branch 'main' of https://github.com/dotnet/runtime into macNw

10138f9

update

082cf40

Merge branch 'main' of https://github.com/dotnet/runtime into macNw

79be6bc

This was referenced Jul 19, 2024

[wasm][mt] fail: System.TypeLoadException: Could not load type of field" #102615

Open

nativeaot\\SmokeTests\\Reflection\\Reflection\\Reflection.cmd fails with Access Violation #105136

Closed

stephentoub reviewed Jul 19, 2024

View reviewed changes

dotnet-policy-service bot closed this Aug 18, 2024

wfurt reopened this Aug 19, 2024

dotnet-policy-service bot closed this Sep 18, 2024

filipnavara reopened this Sep 18, 2024

Merge branch 'main' of https://github.com/dotnet/runtime into macNw

cb8e52b

This was referenced Sep 25, 2024

slow macOS - "##[error]The job running on agent Azure Pipelines 9 ran longer than the maximum time of 60 minutes." dotnet/dnceng#1883

Open

failure in Wasm.Build.Tests.WasmTemplateTests.RunWithDifferentAppBundleLocations #108107

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support of TLS 1.3 on macOS POC #104835

add support of TLS 1.3 on macOS POC #104835

wfurt commented Jul 12, 2024

dotnet-policy-service bot commented Jul 12, 2024

liveans Jul 15, 2024 •

edited

Loading

wfurt Jul 15, 2024

rzikm left a comment

rzikm Jul 15, 2024

liveans Jul 15, 2024 •

edited

Loading

rzikm Jul 15, 2024

rzikm Jul 15, 2024

rzikm Jul 15, 2024

wfurt commented Jul 15, 2024

stephentoub Jul 18, 2024

stephentoub Jul 18, 2024

stephentoub Jul 19, 2024

stephentoub Jul 19, 2024

stephentoub Jul 19, 2024

stephentoub Jul 19, 2024

stephentoub Jul 19, 2024

stephentoub Jul 19, 2024

stephentoub Jul 19, 2024

stephentoub Jul 19, 2024

stephentoub Jul 19, 2024

dotnet-policy-service bot commented Aug 18, 2024

dotnet-policy-service bot commented Sep 18, 2024

		@@ -0,0 +1,429 @@
		// Licensed to the .NET Foundation under one or more agreements.
		// The .NET Foundation licenses this file to you under the MIT license.

		[LibraryImport(Interop.Libraries.AppleCryptoNative, EntryPoint = "AppleCryptoNative_NwCancelConnection")]
		internal static partial int NwCancelConnection(SafeSslHandle sslHandle);

add support of TLS 1.3 on macOS POC #104835

Are you sure you want to change the base?

add support of TLS 1.3 on macOS POC #104835

Conversation

wfurt commented Jul 12, 2024

dotnet-policy-service bot commented Jul 12, 2024

liveans Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rzikm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liveans Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wfurt commented Jul 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dotnet-policy-service bot commented Aug 18, 2024

dotnet-policy-service bot commented Sep 18, 2024

liveans Jul 15, 2024 •

edited

Loading

liveans Jul 15, 2024 •

edited

Loading