Add server-side SNI support #28278

krwq · 2018-03-20T22:23:26Z

Related issue: https://github.com/dotnet/corefx/issues/24553
This is for overview only so that it is easier to discuss the API shape.

Changes to original proposal:

delegate is defined in the namespace and not in the SslStream (to match others)
name has changed to ServerCertificateSelectionCallback
removed X509CertificateCollection from return values as currently do not see what it would be used for (if other implementations require it will change it)

Left:

https://github.com/dotnet/corefx/issues/28585 client on windows does not seem to be IDN mapping before sending. current version supports both but need to revalidate with the spec
document what is known during the callback

cc @Drawaes

krwq · 2018-03-20T22:24:52Z

src/System.Net.Security/tests/FunctionalTests/SslStreamSNITest.cs

@@ -0,0 +1,111 @@
+using System;


(pending) missing copyright header

krwq · 2018-03-20T22:26:11Z

src/System.Net.Security/ref/System.Net.Security.cs

@@ -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 X509Certificate ServerCertificateSelectionCallback(object sender, string hostName);


Let's treat this file as informal API design discussion and other files as reference

ref definitions need fully qualified type name: "System.Security.Cryptography.X509Certificates.X509Certificate" like the delegate definition above.

krwq · 2018-03-20T22:26:38Z

src/System.Net.Security/src/PinvokeAnalyzerExceptionList.analyzerdata

@@ -0,0 +1 @@
+schannel.dll!SslGetServerIdentity


I think we might be missing this somewhere - UAP perhaps?

remove this

davidsh

Great work so far!

We can talk offline about how to set up a working end-to-end test (manual only....not for CI).

davidsh · 2018-03-20T22:53:46Z

src/Common/src/Interop/Windows/Interop.Libraries.cs

@@ -23,6 +23,7 @@ internal static partial class Libraries
        internal const string PerfCounter = "perfcounter.dll";
        internal const string RoBuffer = "api-ms-win-core-winrt-robuffer-l1-1-0.dll";
        internal const string Secur32 = "secur32.dll";
+        internal const string SChannel = "schannel.dll";


Pls investigate if you can use 'secur32.dll' for this p/invoke.

Seems like only in schannel.dll, getting EntryPointNotFoundException with secur32

davidsh · 2018-03-20T22:54:26Z

src/Common/tests/System/Net/VirtualNetwork/VirtualNetwork.cs

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


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

davidsh · 2018-03-20T22:54:58Z

src/Common/tests/System/Net/VirtualNetwork/VirtualNetwork.cs

+        public void BreakConnection()
+        {
+            _connectionBroken = true;
+            _serverDataAvailable.Release(1000000);


nit: you could use "_" in numeric literals to make this easier to read.

davidsh · 2018-03-20T22:55:38Z

src/System.Net.Security/ref/System.Net.Security.cs

@@ -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 X509Certificate ServerCertificateSelectionCallback(object sender, string hostName);


ref definitions need fully qualified type name: "System.Security.Cryptography.X509Certificates.X509Certificate" like the delegate definition above.

davidsh · 2018-03-20T22:55:58Z

src/System.Net.Security/ref/System.Net.Security.cs

@@ -179,6 +182,7 @@ public partial class SslStream : AuthenticatedStream
        public virtual void AuthenticateAsServer(System.Security.Cryptography.X509Certificates.X509Certificate serverCertificate) { }
        public virtual void AuthenticateAsServer(System.Security.Cryptography.X509Certificates.X509Certificate serverCertificate, bool clientCertificateRequired, System.Security.Authentication.SslProtocols enabledSslProtocols, bool checkCertificateRevocation) { }
        public virtual void AuthenticateAsServer(System.Security.Cryptography.X509Certificates.X509Certificate serverCertificate, bool clientCertificateRequired, bool checkCertificateRevocation) { }
+        public void AuthenticateAsServer(SslServerAuthenticationOptions sslServerAuthenticationOptions) { }


we don't add new sync-only APIs. so, pls remove this.

davidsh · 2018-03-20T22:57:58Z

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

@@ -334,14 +351,16 @@ public virtual void AuthenticateAsServer(X509Certificate serverCertificate, bool
            AuthenticateAsServer(options);
        }

-        private void AuthenticateAsServer(SslServerAuthenticationOptions sslServerAuthenticationOptions)
+        public void AuthenticateAsServer(SslServerAuthenticationOptions sslServerAuthenticationOptions)


should be left as 'private'

davidsh · 2018-03-20T22:58:33Z

src/System.Net.Security/src/System/Net/Security/SslStreamPal.Windows.cs

+                    return null;
+                }
+
+                return Encoding.UTF8.GetString(serverIdentityBytes, serverIdentitySize);


Pls verify RFC for TLS/SSL that it is UTF8 encoding.

still pending

@davidsh
RFC 4366 defines UTF-8 (should only be used with host names which are defined to be UTF-8)
RFC 6066 (App. A) overrides it to ASCII encoding for both host name and SNI. We should stick to whatever we do for host name (ASCII? - need to check this)

slightly confused on the code -not seeing any other encoding than utf-8 in our code base and targetName for client side seems to be delegated to SspiCli - we should check somehow what that does

I believe that it is correct at one stage utf8 was okay. But as ASCII is the one true way now for DNS names (with internationalization being supported via App A) and this isn't an existing feature you shouldn't provide backwards compat. Although I do wonder if you should just surface a Span for checking anyway

I agree that the string returned from the API should be "decoded" meaning it should undo any Punnycode if it was transmitted that way on the wire.

@Tratcher: I have mixed feelings about this. From dev perspective it is always easier to do less. From perf perspective it is better to not create a copy of the string (the name is already allocated inside client hello) and comparing can be done with byte comparison (assuming names are in the same encoding which should be the case). From TLS spec standpoint it probably deoesn't matter as it is simply a byte array and length and encoding is an implementation detail (probably that's why they are being vague about what should implementation do about it)

You're not avoiding any work in the end, you're only punting it to the caller.

You kinda are. If you have a server name you would eacape it 1x on the server. Then when a name comes in from the client you do a byte compare. You would likely do the initial escape once and the compare for n number of connections. If you have to depunifiy© the client to compare to a string everytime then you are doing n units of work.

Fair, but it requires the caller to be IDN aware.

davidsh · 2018-03-20T22:59:30Z

src/System.Net.Security/tests/FunctionalTests/SslStreamSNITest.cs

+        [Theory]
+        [InlineData("a")]
+        [InlineData("test")]
+        [InlineData("aaaaaaaaaaaaaaaaaaaabbbbbbbbbbbbbbbbbbbbbbbbbbbbccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc")]


Might be easier to read and maintain if this used memberdata and a "yield return" with constructing the large strings as "new String(a, 20) + etc." for repetitions.

davidsh · 2018-03-20T22:59:56Z

src/System.Net.Security/tests/FunctionalTests/SslStreamSNITest.cs

+        [InlineData("a")]
+        [InlineData("test")]
+        [InlineData("aaaaaaaaaaaaaaaaaaaabbbbbbbbbbbbbbbbbbbbbbbbbbbbccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc")]
+        public void ClientSendsSNIServerReceivesIt(string hostName)


Pls use current test naming convention (3 parts with underscores). Pls see other tests for examples.

davidsh · 2018-03-20T23:00:20Z

src/System.Net.Security/tests/FunctionalTests/System.Net.Security.Tests.csproj

@@ -22,6 +22,7 @@
  <ItemGroup>
    <Compile Include="NotifyReadVirtualNetworkStream.cs" />
    <Compile Include="DummyTcpServer.cs" />
+    <Compile Include="SslStreamSNITest.cs" />


Pls move this line lower into the rest of the SslStream includes.

davidsh

Left some comments.

krwq · 2018-03-20T23:32:00Z

src/System.Net.Security/tests/FunctionalTests/SslStreamSNITest.cs

+                };
+
+                var cts = new CancellationTokenSource();
+                Assert.ThrowsAsync<NotSupportedException>(async () => {


Open question is what should we throw when callback returns null - currently there is a code handling null case and I have not added anything new but this is something we might consider changing.

What does SslStream throw today if you try to AuthenticateAsServerAsync with a null certificate? whatever that does is probably going to make most sense?

this scenario is being detected early - in this case we don't know about null until we receive hello from the client

We need a test for this to make sure that the exception we might throw if catch-able by the developer's code. We don't want, for instance, to throw an exception on a threadpool thread.

@davidsh this is a test for that - I will likely change the exception once I figure out which one is the most appropriate here.

Tratcher · 2018-03-22T22:09:59Z

src/System.Net.Security/src/System/Net/Security/SslStreamPal.Windows.cs

+                int ret = Interop.SChannel.SslGetServerIdentity(clientHelloBytes, clientHello.Length, out serverIdentityBytes, out serverIdentitySize, 0);
+                if (ret != 0 || serverIdentitySize == 0)
+                {
+                    return null;


So the callback will be invoked with a null host name if SNI was not provided by the client?

Having a null string as input to the developer's callback seems reasonable in this case.

@Tratcher correct

please review new iteration

krwq · 2018-03-26T19:58:27Z

src/System.Net.Security/src/System/Net/Security/SNIHelper.cs

+        //   - NameType name_type (1 byte) => 0x00 is host_name
+        //   - opaque HostName
+        // Per spec:
+        //   If the hostname labels contain only US-ASCII characters, then the


this part is TODO. We will likely decide on either returning bytes or IDN unmap but let's wait for API design discussion - for now treating as UTF-8 string

krwq · 2018-03-26T20:01:29Z

src/Common/tests/System/Net/VirtualNetwork/VirtualNetwork.cs

        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;


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)

davidsh

Left some comments.

davidsh · 2018-03-26T20:33:52Z

src/System.Net.Security/src/System/Net/Security/SNIHelper.cs

+
+namespace System.Net.Security
+{
+    internal class SNIHelper


nit: SNIHelper -> SniHelper
Style guideline uses normal Pascal casing when acronym is more than 2 characters. Change filename as well to match.

davidsh · 2018-03-26T20:37:53Z

src/System.Net.Security/tests/FunctionalTests/SslStreamSNITest.cs

+{
+    using Configuration = System.Net.Test.Common.Configuration;
+
+    public class SslStreamSNITest


nit: "SslStreamSNITest" -> "SslStreamSniTest".

..change all other "SNI" patterns to "Sni" in other file(s).

davidsh · 2018-03-26T20:43:16Z

src/System.Net.Security/src/System/Net/Security/SNIHelper.cs

+        //   - opaque fragment[SSLPlaintext.length]
+        private static string GetSniFromSslPlainText(ReadOnlySpan<byte> sslPlainText)
+        {
+            // Is SSL 3 handshake? SSL 2 does not support extensions - skipping as well


We currently don't "allow" Ssl2 or Ssl3 in .NET Core. It's marked as Obsolete.
https://github.com/dotnet/corefx/blob/master/src/System.Net.Primitives/ref/System.Net.Primitives.cs#L494

But I agree we should probably be correct in parsing it in case it comes from the server.

davidsh · 2018-03-26T20:48:50Z

src/System.Net.Security/tests/FunctionalTests/SslStreamSNITest.cs

+        }
+
+        [Fact]
+        public void SslStream_UnknownHostName_Fails()


Might be better to rename this test to indicate that it is testing that no SNI data was sent from client to server and thus the callback returns null.

So, perhaps this is a better name: "SslStream_NoSniFromClient_CallbackReturnsNull"

@davidsh, thanks, I thought the last part is always Fails or Ok 😄

The last part is usually "Success" in cases where it makes sense. Some folks use "Succeeds". But "Success" is used most often in the repo especially for network tests.

For failure cases, it always better to use terms like "Throws" or "ThrowsSocketException", or in this case, "CallbackReturnsNull". It is more descriptive and helps us understand the test case better.

Thanks will update!

krwq · 2018-03-26T21:08:59Z

src/System.Net.Security/src/System/Net/Security/SNIHelper.cs

+        // Opaque type is of structure:
+        //   - length (minimum number of bytes to hold the max value)
+        //   - data (length bytes)
+        // We will only use opaque bytes which are of max size: 255 (length = 1) or 2^16-1 (length = 2).


nit: opaque types (not bytes)

krwq · 2018-03-26T21:10:20Z

src/System.Net.Security/src/System/Net/Security/SNIHelper.cs

+        }
+
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        private static ushort ReadUint16(ReadOnlySpan<byte> bytes)


actually we could leave it as int since we have to double convert ushort otherwise (here and for comparison)

krwq · 2018-03-26T22:34:11Z

cc: @davidfowl

danmoseley · 2018-03-27T17:53:24Z

src/System.Net.Security/src/System/Net/Security/SNIHelper.cs

+                return null;
+            }
+
+            return GetSniFromSslHandshake(sslHandshake);


Nit, might be simpler to just combine the methods

Then you don't need to check sslHandshake.Length < 4 I think.

@danmosemsft, could you expand? I think I will still need to check - note that sslHandshake is sliced sslPlaintext. The lengths are fairly redundant in the SSL spec which makes this code slightly confusing but allow to skip parts implementation doesn't understand

You are right. I misread.

krwq · 2018-03-27T19:24:53Z

src/System.Net.Security/src/System/Net/Security/SslStreamPal.Windows.cs

@@ -10,6 +10,7 @@
 using System.Security.Authentication.ExtendedProtection;
 using System.Security.Cryptography.X509Certificates;
 using System.Security.Principal;
+using System.Text;


GrabYourPitchforks

Minor feedback, still approved.

GrabYourPitchforks · 2018-03-29T23:42:16Z

src/System.Net.Security/src/System/Net/Security/SniHelper.cs

@@ -15,6 +14,7 @@ internal class SniHelper
        private const int UInt24Size = 3;
        private const int RandomSize = 32;
        private static IdnMapping s_idnMapping = CreateIdnMapping();
+        private static Encoding s_encoding = new UTF8Encoding(encoderShouldEmitUTF8Identifier: false, throwOnInvalidBytes: true);


Nit: private readonly static.

krwq · 2018-03-29T23:54:06Z

Thank you @GrabYourPitchforks! I've removed the dependency I added by creating Utf8Encoding differently (thank you @tarekgh!) and added a test to make sure we do not regress that in the future

karelz · 2018-03-30T00:40:22Z

@stephentoub @bartonjs @Drawaes @Tratcher any additional feedback before we merge the PR?

Tratcher · 2018-03-30T03:10:44Z

src/System.Net.Security/src/System/Net/Security/SslAuthenticationOptions.cs

@@ -66,6 +67,7 @@ internal SslAuthenticationOptions(SslServerAuthenticationOptions sslServerAuthen
        internal bool CheckCertName { get; set; }
        internal RemoteCertValidationCallback CertValidationDelegate { get; set; }
        internal LocalCertSelectionCallback CertSelectionDelegate { get; set; }
+        internal ServerCertCallback ServerCertSelectionDelegate { get; set; }


Missed this one?

krwq · 2018-03-30T03:45:49Z

CI's being a little slow...

krwq · 2018-03-30T06:59:54Z

test OSX x64 Debug Build
test Linux x64 Release Build
test CROSS Check

krwq · 2018-03-30T15:41:51Z

@karelz seems like all PRs are timing out on these 3 builds. CI was passing on almost identical version such as 40f8447 - do you think it is ok to merge?

karelz · 2018-03-30T16:39:58Z

Let's wait on CI to come back for now. Having the fix in master just few hours earlier is not worth the risk of breaking master just before last integration to release/2.1 branch.
We will have to come up with overall strategy for CI impact on ZBB and our integration plans.

stephentoub · 2018-03-30T16:43:29Z

src/System.Net.Security/src/System/Net/Security/SniHelper.cs

+
+        private static Encoding CreateEncoding()
+        {
+            return Encoding.GetEncoding("utf-8", new EncoderExceptionFallback(), new DecoderExceptionFallback());


Why does this need to use GetEncoding("utf-8", ...) rather than new UTF8Encoding() or Encoding.UTF8?

@stephentoub Utf8Encoding is in different assembly and it would introduce new dependency (System.Text.Encoding.Extensions) where we can create this exact type without introducing it by doing above

Is that a dependency we care about? The implementation of UTF8Encoding is in corelib. And using Encoding.GetEncoding(string) here would seem to a) be more expensive, and b) not necessarily produce a UTF8Encoding as (if I'm understanding the code correctly) it'll go through any registered providers before falling back to looking up what encoder should be used by internal tables.

(And I assume Encoding.UTF8 can't be used because the fallbacks it needs are different?)

cc: @tarekgh, @weshaggard

Anyway, don't block on this one.

Yes, Encoding.UTF8 does not throw on invalid utf-8 byte sequences.

The more expensive part I'm not concerned about since this is once per process cost.

Getting non-utf8 here would be a concern here though. I'm not sure if there are any concerns with new dependency but it seemed not appropriate to add it just before 2.1 release.

krwq · 2018-03-30T20:07:32Z

test CROSS Check

krwq · 2018-03-30T20:13:45Z

@karelz CROSS Check shows as "Started." but CI console text says it has succeeded: https://ci.dot.net/job/dotnet_corefx/job/master/job/CROSS_check_prtest/14394/consoleText I think this is ok to merge.

karelz · 2018-03-30T20:14:40Z

Sounds good, go ahead. Thanks for pushing it through!

stephentoub · 2018-03-30T20:15:40Z

Was there an answer to my question at #28278 (comment)?

krwq · 2018-03-30T20:20:45Z

@stephentoub I've missed the question. Yes, it is mutating but this is what we already do for both client and server. Examples:
https://github.com/krwq/corefx/blob/705e95f02d179cccc6a290e5cf9f05fbd9873ed3/src/System.Net.Security/src/System/Net/Security/SslStream.cs#L252
https://github.com/krwq/corefx/blob/705e95f02d179cccc6a290e5cf9f05fbd9873ed3/src/System.Net.Security/src/System/Net/Security/SslStream.cs#L323
https://github.com/krwq/corefx/blob/705e95f02d179cccc6a290e5cf9f05fbd9873ed3/src/System.Net.Security/src/System/Net/Security/SslStream.cs#L360

I've created an issue to track this: https://github.com/dotnet/corefx/issues/28657

krwq · 2018-03-30T21:00:30Z

Merging this since https://github.com/dotnet/corefx/issues/28657 is marked as 2.1. Thanks everyone!

dotnet/runtime#17677 dotnet/corefx#28278

* add initial implementation for server side SNI support * remove analyzerdata * add braces around single line if statements and inline simple functions * fix deadlock on linux * add test with special characters * IDN decoding * idn unmapping (with fallback) * apply feedback, fix netfx/uwp * rename the SNIHelper to SniHelper * rename test file as well (git does not like renames on windows) * change file casing in csproj * test should expect AuthenticationException * add SniHelper tests * apply review feedback * shorten SNI (limit is 63 bytes) * replace remaining constants * test behavior on truncated client hello * add structures descriptions * apply review feedback * get rid of the new dependency and add a test for invalid utf-8 bytes Commit migrated from dotnet/corefx@2d39212

krwq requested review from stephentoub and davidsh March 20, 2018 22:23

krwq added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Mar 20, 2018

krwq commented Mar 20, 2018

View reviewed changes

davidsh added the area-System.Net.Security label Mar 20, 2018

davidsh reviewed Mar 20, 2018

View reviewed changes

davidsh previously requested changes Mar 20, 2018

View reviewed changes

krwq commented Mar 20, 2018

View reviewed changes

krwq changed the title ~~[WIP] Add server sni windows~~ [WIP] Add server-side SNI support (currently windows only) Mar 20, 2018

karelz assigned krwq Mar 21, 2018

Tratcher reviewed Mar 22, 2018

View reviewed changes

Tratcher mentioned this pull request Mar 23, 2018

Add SNI Support aspnet/KestrelHttpServer#2425

Merged

add initial implementation for server side SNI support

ba4b048

krwq force-pushed the add_server_sni_windows branch from aab344a to ba4b048 Compare March 26, 2018 19:11

krwq added 2 commits March 26, 2018 12:11

remove analyzerdata

baa3c5a

add braces around single line if statements and inline simple functions

d7d9c3c

krwq commented Mar 26, 2018

View reviewed changes

krwq changed the title ~~[WIP] Add server-side SNI support (currently windows only)~~ [WIP] Add server-side SNI support Mar 26, 2018

davidsh reviewed Mar 26, 2018

View reviewed changes

krwq commented Mar 26, 2018

View reviewed changes

karelz added this to the 2.1.0 milestone Mar 27, 2018

danmoseley reviewed Mar 27, 2018

View reviewed changes

krwq commented Mar 27, 2018

View reviewed changes

fix deadlock on linux

d056d54

GrabYourPitchforks approved these changes Mar 29, 2018

View reviewed changes

get rid of the new dependency and add a test for invalid utf-8 bytes

3e69a17

krwq removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Mar 29, 2018

bartonjs approved these changes Mar 30, 2018

View reviewed changes

Tratcher approved these changes Mar 30, 2018

View reviewed changes

krwq mentioned this pull request Mar 30, 2018

[2.1] Add server-side SNI support #28646

Closed

stephentoub reviewed Mar 30, 2018

View reviewed changes

krwq merged commit 2d39212 into dotnet:master Mar 30, 2018

FiniteReality mentioned this pull request May 18, 2018

SSL-SNI requirement causes failed voice websocket connects in Brazil region discord-net/Discord.Net#1064

Closed

lmarszal mentioned this pull request May 30, 2018

Fix PlatformNotSupportedException when running in docker on Linux justcoding121/titanium-web-proxy#454

Closed

3 tasks

This was referenced Jan 31, 2020

Investigate IDN mapping rules when parsing/writing server name dotnet/runtime#25668

Closed

SslStream mutates state of Client/ServerAuthenticationOptions dotnet/runtime#25692

Closed

ryanerdmann mentioned this pull request Jan 31, 2020

Asynchronous SNI API for SslStream dotnet/runtime#27350

Closed

kiminuo added a commit to kiminuo/WalletWasabi that referenced this pull request Sep 23, 2020

TorHttpClient: SslStream is supposed to support SNI since .NET Core 2.1.

603de2c

dotnet/runtime#17677 dotnet/corefx#28278

kiminuo mentioned this pull request Sep 23, 2020

TorHttpClient: SslStream is supposed to support SNI since .NET Core 2.1. zkSNACKs/WalletWasabi#4428

Merged

molnard pushed a commit to molnard/WalletWasabi that referenced this pull request Oct 22, 2020

TorHttpClient: SslStream is supposed to support SNI since .NET Core 2.1.

fe71858

dotnet/runtime#17677 dotnet/corefx#28278

wfurt mentioned this pull request Feb 15, 2022

Put generated InvalidClientHelloDataTruncatedBytes data into one test dotnet/runtime#62480

Closed

		@@ -0,0 +1 @@
		schannel.dll!SslGetServerIdentity

Add server-side SNI support #28278

Add server-side SNI support #28278

Conversation

krwq commented Mar 20, 2018 • edited

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

davidsh left a 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

krwq Mar 20, 2018 • edited

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

davidsh left a 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

krwq Mar 22, 2018 • edited

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

davidsh left a 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

davidsh Mar 26, 2018 • edited

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

krwq commented Mar 26, 2018

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

GrabYourPitchforks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krwq commented Mar 29, 2018

karelz commented Mar 30, 2018

Choose a reason for hiding this comment

krwq commented Mar 30, 2018

krwq commented Mar 30, 2018

krwq commented Mar 30, 2018

karelz commented Mar 30, 2018

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

krwq commented Mar 30, 2018

krwq commented Mar 30, 2018

karelz commented Mar 30, 2018

krwq commented Mar 20, 2018 •

edited

krwq Mar 20, 2018 •

edited

krwq Mar 22, 2018 •

edited

davidsh Mar 26, 2018 •

edited

krwq commented Mar 30, 2018 •

edited