-
Notifications
You must be signed in to change notification settings - Fork 5k
Add server-side SNI support #28278
Add server-side SNI support #28278
Changes from 12 commits
ba4b048
baa3c5a
d7d9c3c
d056d54
7d9395e
7537db0
1b8c11d
df0bf87
353bf99
4b63460
bd92873
705e95f
c2e31f9
16015e0
2432662
49f462e
3dda001
40f8447
fe8c956
3e69a17
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
private bool _connectionBroken = false; | ||
|
||
public void ReadFrame(bool server, out byte[] buffer) | ||
{ | ||
if (_connectionBroken) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
||
|
@@ -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; | ||
|
@@ -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; | ||
|
||
|
@@ -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); | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @stephentoub - The code has been API reviewed. |
||
|
||
public partial class NegotiateStream : AuthenticatedStream | ||
{ | ||
public NegotiateStream(System.IO.Stream innerStream) : base(innerStream, false) { } | ||
|
@@ -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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it might be possible to use Span here instead of byte[] There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
@@ -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."); | ||
|
@@ -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); | ||
} | ||
|
||
|
There was a problem hiding this comment.
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)