Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Add server-side SNI support #28278

Merged
merged 20 commits into from
Mar 30, 2018
Merged

Add server-side SNI support #28278

Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
ba4b048
add initial implementation for server side SNI support
krwq Mar 20, 2018
baa3c5a
remove analyzerdata
krwq Mar 26, 2018
d7d9c3c
add braces around single line if statements and inline simple functions
krwq Mar 26, 2018
d056d54
fix deadlock on linux
krwq Mar 27, 2018
7d9395e
add test with special characters
krwq Mar 27, 2018
7537db0
IDN decoding
krwq Mar 27, 2018
1b8c11d
idn unmapping (with fallback)
krwq Mar 27, 2018
df0bf87
apply feedback, fix netfx/uwp
krwq Mar 27, 2018
353bf99
rename the SNIHelper to SniHelper
krwq Mar 27, 2018
4b63460
rename test file as well (git does not like renames on windows)
krwq Mar 27, 2018
bd92873
change file casing in csproj
krwq Mar 27, 2018
705e95f
test should expect AuthenticationException
krwq Mar 27, 2018
c2e31f9
add SniHelper tests
krwq Mar 28, 2018
16015e0
apply review feedback
krwq Mar 29, 2018
2432662
shorten SNI (limit is 63 bytes)
krwq Mar 29, 2018
49f462e
replace remaining constants
krwq Mar 29, 2018
3dda001
test behavior on truncated client hello
krwq Mar 29, 2018
40f8447
add structures descriptions
krwq Mar 29, 2018
fe8c956
apply review feedback
krwq Mar 29, 2018
3e69a17
get rid of the new dependency and add a test for invalid utf-8 bytes
krwq Mar 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
35 changes: 34 additions & 1 deletion src/Common/tests/System/Net/VirtualNetwork/VirtualNetwork.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,29 @@ namespace System.Net.Test.Common
{
public class VirtualNetwork
{
public class VirtualNetworkConnectionBroken : Exception
{
public VirtualNetworkConnectionBroken() : base("Connection broken") { }
}

private readonly int WaitForReadDataTimeoutMilliseconds = 30 * 1000;

private readonly ConcurrentQueue<byte[]> _clientWriteQueue = new ConcurrentQueue<byte[]>();
private readonly ConcurrentQueue<byte[]> _serverWriteQueue = new ConcurrentQueue<byte[]>();

private readonly SemaphoreSlim _clientDataAvailable = new SemaphoreSlim(0);
private readonly SemaphoreSlim _serverDataAvailable = new SemaphoreSlim(0);

public bool DisableConnectionBreaking { get; set; } = false;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reconsider if this option is needed (I think there was 1 test case on linux failing because of lack of this but I think I accidentally reverted the fix - let's wait for linux results)

private bool _connectionBroken = false;

public void ReadFrame(bool server, out byte[] buffer)
{
if (_connectionBroken)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: style - we usually use braces around the body of 'if' statements unless the whole thing is on a single line.

{
throw new VirtualNetworkConnectionBroken();
}

SemaphoreSlim semaphore;
ConcurrentQueue<byte[]> packetQueue;

Expand All @@ -39,6 +52,11 @@ public void ReadFrame(bool server, out byte[] buffer)
throw new TimeoutException("VirtualNetwork: Timeout reading the next frame.");
}

if (_connectionBroken)
{
throw new VirtualNetworkConnectionBroken();
}

bool dequeueSucceeded = false;
int remainingTries = 3;
int backOffDelayMilliseconds = 2;
Expand All @@ -62,6 +80,11 @@ public void ReadFrame(bool server, out byte[] buffer)

public void WriteFrame(bool server, byte[] buffer)
{
if (_connectionBroken)
{
throw new VirtualNetworkConnectionBroken();
}

SemaphoreSlim semaphore;
ConcurrentQueue<byte[]> packetQueue;

Expand All @@ -82,5 +105,15 @@ public void WriteFrame(bool server, byte[] buffer)
packetQueue.Enqueue(innerBuffer);
semaphore.Release();
}

public void BreakConnection()
{
if (!DisableConnectionBreaking)
{
_connectionBroken = true;
_serverDataAvailable.Release(1_000_000);
_clientDataAvailable.Release(1_000_000);
}
}
}
}
10 changes: 10 additions & 0 deletions src/Common/tests/System/Net/VirtualNetwork/VirtualNetworkStream.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,5 +156,15 @@ public override Task WriteAsync(byte[] buffer, int offset, int count, Cancellati

public override void EndWrite(IAsyncResult asyncResult) =>
TaskToApm.End(asyncResult);

protected override void Dispose(bool disposing)
{
if (disposing)
{
_network.BreakConnection();
}

base.Dispose(disposing);
}
}
}
3 changes: 3 additions & 0 deletions src/System.Net.Security/ref/System.Net.Security.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ public enum EncryptionPolicy
RequireEncryption = 0,
}
public delegate System.Security.Cryptography.X509Certificates.X509Certificate LocalCertificateSelectionCallback(object sender, string targetHost, System.Security.Cryptography.X509Certificates.X509CertificateCollection localCertificates, System.Security.Cryptography.X509Certificates.X509Certificate remoteCertificate, string[] acceptableIssuers);
public delegate System.Security.Cryptography.X509Certificates.X509Certificate ServerCertificateSelectionCallback(object sender, string hostName);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has this been API reviewed? I'm wondering if we want to introduce a new delegate type or instead just use Func<...>. Also, why object for sender? What does someone do with that, and should it be more strongly typed?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be more than happy with those changes. The only reason for object and a new delegate is convention nothing else.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stephentoub - The code has been API reviewed. object is not used in our code anywhere but it is a convention we chose in the past for all delegates - Func probably makes more sense but would stick out a little.


public partial class NegotiateStream : AuthenticatedStream
{
public NegotiateStream(System.IO.Stream innerStream) : base(innerStream, false) { }
Expand Down Expand Up @@ -108,6 +110,7 @@ public class SslServerAuthenticationOptions
public X509RevocationMode CertificateRevocationCheckMode { get { throw null; } set { } }
public List<SslApplicationProtocol> ApplicationProtocols { get { throw null; } set { } }
public RemoteCertificateValidationCallback RemoteCertificateValidationCallback { get { throw null; } set { } }
public ServerCertificateSelectionCallback ServerCertificateSelectionCallback { get { throw null; } set { } }
public EncryptionPolicy EncryptionPolicy { get { throw null; } set { } }
}
public partial class SslClientAuthenticationOptions
Expand Down
2 changes: 2 additions & 0 deletions src/System.Net.Security/src/System.Net.Security.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
<Compile Include="System\Net\FixedSizeReader.cs" />
<Compile Include="System\Net\HelperAsyncResults.cs" />
<Compile Include="System\Net\Logging\NetEventSource.cs" />
<Compile Include="System\Net\Security\SniHelper.cs" />
<Compile Include="System\Net\Security\SslApplicationProtocol.cs" />
<Compile Include="System\Net\Security\SslAuthenticationOptions.cs" />
<Compile Include="System\Net\Security\SslClientAuthenticationOptions.cs" />
Expand Down Expand Up @@ -418,6 +419,7 @@
<Reference Include="System.Security.Cryptography.X509Certificates" />
<Reference Include="System.Security.Principal" />
<Reference Include="System.Security.Principal.Windows" />
<Reference Include="System.Text.Encoding.Extensions" />
<Reference Include="System.Threading" />
<Reference Include="System.Threading.Tasks" />
<Reference Include="System.Threading.ThreadPool" />
Expand Down
18 changes: 14 additions & 4 deletions src/System.Net.Security/src/System/Net/Security/SecureChannel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -624,15 +624,26 @@ private bool AcquireClientCredentials(ref byte[] thumbPrint)
//
// Acquire Server Side Certificate information and set it on the class.
//
private bool AcquireServerCredentials(ref byte[] thumbPrint)
private bool AcquireServerCredentials(ref byte[] thumbPrint, byte[] clientHello)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it might be possible to use Span here instead of byte[]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we would have to convert it everywhere to check if there is any gain from that - I'll leave it out of this PR

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unless you mean only for our case but still it shouldn't matter

{
if (NetEventSource.IsEnabled)
NetEventSource.Enter(this);

X509Certificate localCertificate = null;
bool cachedCred = false;

if (_sslAuthenticationOptions.CertSelectionDelegate != null)
if (_sslAuthenticationOptions.ServerCertSelectionDelegate != null)
{
string serverIdentity = SniHelper.GetServerName(clientHello);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SChannel really makes you parse the hello manually? I'd much rather see the whole parser dropped if the implementations will give back the string(s).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Schannel actually has similar API to this one but is not available on Windows 7. Other reason we chose parsing is that certificate is passed to security context constructor where OpenSSL way to get SNI is to set a callback after construction and callback is called when handshake function is called. This is doable but fairly risky. Having parsing allows us to do other things easier (i.e. make cipher suites be available in the callback) and have uniform code across all platforms. Since we already got parsing it doesn't make much sense to do different things on other platforms

localCertificate = _sslAuthenticationOptions.ServerCertSelectionDelegate(serverIdentity);

if (localCertificate == null)
{
throw new AuthenticationException(SR.net_ssl_io_no_server_cert);
}
}
// This probably never gets called as this is a client options delegate
else if (_sslAuthenticationOptions.CertSelectionDelegate != null)
{
X509CertificateCollection tempCollection = new X509CertificateCollection();
tempCollection.Add(_sslAuthenticationOptions.ServerCertificate);
Expand Down Expand Up @@ -744,7 +755,6 @@ private SecurityStatusPal GenerateToken(byte[] input, int offset, int count, ref
#if TRACE_VERBOSE
if (NetEventSource.IsEnabled) NetEventSource.Enter(this, $"_refreshCredentialNeeded = {_refreshCredentialNeeded}");
#endif

if (offset < 0 || offset > (input == null ? 0 : input.Length))
{
NetEventSource.Fail(this, "Argument 'offset' out of range.");
Expand Down Expand Up @@ -786,7 +796,7 @@ private SecurityStatusPal GenerateToken(byte[] input, int offset, int count, ref
if (_refreshCredentialNeeded)
{
cachedCreds = _sslAuthenticationOptions.IsServer
? AcquireServerCredentials(ref thumbPrint)
? AcquireServerCredentials(ref thumbPrint, input)
: AcquireClientCredentials(ref thumbPrint);
}

Expand Down