Cleaning up SslStream, MobileAuthenticatedStream and TaskToApm. (#17393)
commit1553889bc54f87060158febca7e6b8b9910975f8
authorMartin Baulig <mabaul@microsoft.com>
Thu, 24 Oct 2019 17:42:54 +0000 (24 13:42 -0400)
committerAlexander Köplinger <alex.koeplinger@outlook.com>
Thu, 24 Oct 2019 17:42:54 +0000 (24 19:42 +0200)
treeb6565f378f3432404961bcb66e594cb73824feb3
parent638e2c6a8614d25aad591112d3414e28641b97e0
Cleaning up SslStream, MobileAuthenticatedStream and TaskToApm. (#17393)

### Overview

After the removal of the Legacy TLS Provider, we can make some cleanups and improvements to the `SslStream` class.  Since both the remaining providers use the internal `MobileAuthenticatedStream` and support Task-async code, we can get rid of some of the Begin/End async logic and the the underlying
`Stream` class handle it instead.

This also fixes a nasty issue with some of those task objects leaking unobserved exceptions.  The problem was that `SslStream.Dispose()` needs to clear out all resources, but in doing so also nulls out the `Impl` handle to the `MobileAuthenticatedStream` - so a subsequent `End*()` call will not reach it's corresponding `TaskToApm.End()` method.

There are also some cleanups in the internal APIs that were only ever used internally and not intended for public consumption.

### Mono.Security.dll

* `Mono.Security.Interface`: removed internal `ICertificateValidator2` interface.

* `Mono.Security.Interface.IMonoSslStream`: this internal interface is only used by the web-tests, which only use it as a way to get the `SslStream` instance, so all those `Begin/End` async methods are now gone.

* `Mono.Security.Interface.IMonoSslStream2`: removed another internal interface.

* `Mono.Security.Interface.MonoTlsProvider`: removed some `internal abstract` methods; these have been moved into a new `Mono.Net.Security.MobileTlsProvider` inside `System.dll`.

### System.dll

* `Mono.Net.Security.MobileTlsProvider`:
  New abstract internal base class, which received the aforementioned abstract internal methods.  The naming is such to make it consistent with the already existing `MobileTlsContext` and `MobileAuthenticatedStream`.

Since both `AppleTlsProvider` and `BtlsProvider` now have an abstract internal base class inside `System.dll`, they can use the `ChainValidationHelper` class directly and we can also avoid some `Mono.Security` dances like for instance that `MonoSslPolicyErrors` enum.

This will also help make the code easier to read and maintainer.

* `Mono.AppleTls.AppleTlsProvider` and `Mono.Btls.MonoBtlsProvider`: change base class into `MobileTlsProvider` and reflect above mentioned overload changes.

* `Mono.AppleTls.AppleCertificateHelper`: use `ChainValidationHelper` as well.

* `Mono.Net.Security.ChainValidationHelper`: only implement `ICertificateValidator`.
  The `ICertificateValidator` interface is still required because it is in use by the web-tests, but the previous `ICertificateValidator2` was an internal dance.

* `Mono.Net.Security.MobileAuthenticatedStream`: largely cleaned up this class.
  - we now implement the current slimmer version of `IMonoSslStream`.
  - moved all those overloads into `SslStream` that only fill in default values for some of their arguments and then call another overloaded version of their own.
  - removed lots of `Begin/End` methods that are not used anymore.

* `Mono.Net.Security.MonoTlsProviderFactory`: use `MobileTlsStream` everywhere.

* `Mono.Net.Security.MonoTlsStream`: cleanup `SslStream` creation logic; instead of going throw `provider.CreateSslStream()`, we can now use an internal constructor directly, thus avoiding the `IMonoSslStream` interface while also making the code a lot easier to read and understand.

* `System.Net.Mail.SmtpClient`: same here.

* `System.Net.HttpWebRequest`: use `MobileTlsProvider`.

* `System.Net.Security.SslStream`: largely cleaned up this class.
  - we now use `MobileAuthenticatedStream` directly instead of the `IMonoSslStream` interface and therefore could also get rid of `IMonoSslStream2`.
  - cleanup constructor logic to allow the internal constructor being used with a `MobileTlsProvider` instead of going through `provider.CreateSslStream()`.
  - all those overloads that are only filling in default values for some of their arguments before calling another overloaded version of their own now live here instead of in the `MobileAuthenticatedStream` class.  The reason is to simply that `MobileAuthenticatedStream` is already quite a complex and difficult class, and removing some of that complexity out of it will make it easier to understand.
  - handle the `TaskToApm.Begin()` / `TaskToApm.End()` here to ensure that we are not leaking any task objects with unobserved exceptions.
  - removed `BeginRead()` and `BeginWrite()` overloads to let the underlying `Stream` class handle these.

* Bump API snapshot submodule

* [csproj] Update project files
22 files changed:
external/api-snapshot
mcs/class/Mono.Security/Mono.Security.Interface/CertificateValidationHelper.cs
mcs/class/Mono.Security/Mono.Security.Interface/IMonoSslStream.cs
mcs/class/Mono.Security/Mono.Security.Interface/MonoTlsProvider.cs
mcs/class/System/Mono.AppleTls/AppleCertificateHelper.cs
mcs/class/System/Mono.AppleTls/AppleTlsProvider.cs
mcs/class/System/Mono.AppleTls/AppleTlsStream.cs
mcs/class/System/Mono.Btls/MonoBtlsProvider.cs
mcs/class/System/Mono.Btls/MonoBtlsStream.cs
mcs/class/System/Mono.Btls/X509PalImpl.Btls.cs
mcs/class/System/Mono.Net.Security/ChainValidationHelper.cs
mcs/class/System/Mono.Net.Security/MobileAuthenticatedStream.cs
mcs/class/System/Mono.Net.Security/MobileTlsContext.cs
mcs/class/System/Mono.Net.Security/MobileTlsProvider.cs [new file with mode: 0644]
mcs/class/System/Mono.Net.Security/MonoTlsProviderFactory.cs
mcs/class/System/Mono.Net.Security/MonoTlsStream.cs
mcs/class/System/Mono.Net.Security/NoReflectionHelper.cs
mcs/class/System/System.Net.Mail/SmtpClient.cs
mcs/class/System/System.Net.Security/SslStream.cs
mcs/class/System/System.Net/HttpWebRequest.cs
mcs/class/System/System.csproj
mcs/class/System/common_networking.sources