Investigate IDN mapping rules when parsing/writing server name

[krwq]

SslStream Windows implementation does not map special characters when sending host name in the client hello extension and sends utf-8 characters instead.

Note: This does not affect HttpClient scenario which does IDN mapping higher up the stack, only SslStream scenario is affected.

SSL protocol recommends that host name should be send by a client. Host name should be send using one of the client hello extensions. Recommended data flow is following:

[CLIENT] ---> Possibly non-ASCII string --- IDN MAPPING ---> ASCII string ---> UTF8 BYTES ---> ASCII string --- IDN UNMAPPING ---> Possibly non-ASCII string ---> [SERVER logic]

Current windows Implementation seems to be skipping IDN mapping and does following instead:

[CLIENT] ---> Possibly non-ASCII string  ---> UTF8 BYTES ---> ASCII string --- IDN UNMAPPING ---> Possibly non-ASCII string ---> [SERVER logic]

Recently merged server logic implementation (dotnet/corefx#28278) does following to mitigate the problem:

UTF8 BYTES ---> Possibly non-ASCII string or IDN mapped string ---> IDN UNMAPPING ---> Succeeded? ---> Possibly non-ASCII string ---> [SERVER logic]
                                          |                                              |                   ^
                                          |               (See: SniHelper.DecodeString)  | NO                |
                                          +---------------------------------------------->-------------------+

To sum up:

• client on Windows does not seem to be doing IDN mapping before sending (investigate if this is correct and if not fix)
• investigate if described above fallback is ok or if it should return null instead (with the current client implementation no fallback means we can't roundrip non-ascii characters)

Related test case (passes with the fallback):
https://github.com/dotnet/corefx/pull/28278/files#diff-dbaea2525913714cf555096c8af14a03R21
with following test data:
https://github.com/dotnet/corefx/pull/28278/files#diff-dbaea2525913714cf555096c8af14a03R132

On the OpenSSL side we do the conversion explicitly:
https://github.com/dotnet/corefx/blob/c533892f2e57940ec9e66616288bf340b75a9217/src/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs#L129

while on Windows we seem to be passing directly:
https://github.com/dotnet/corefx/blob/c533892f2e57940ec9e66616288bf340b75a9217/src/Common/src/Interop/Windows/sspicli/SecuritySafeHandles.cs#L595

2.0.0 code also does the same thing: https://github.com/dotnet/corefx/blob/release/2.0.0/src/Common/src/Interop/Windows/sspicli/SecuritySafeHandles.cs#L596

Windows documentation doesn't seem to be specifying if it does the conversion for you or not (https://msdn.microsoft.com/en-us/library/windows/desktop/aa375924(v=vs.85).aspx):

pszTargetName [in, optional]

    A pointer to a null-terminated string that uniquely identifies the target server. Schannel uses this value to verify the server certificate. Schannel also uses this value to locate the session in the session cache when reestablishing a connection. The cached session is used only if all of the following conditions are met:

        The target name is the same.
        The cache entry has not expired.
        The application process that calls the function is the same.
        The logon session is the same.
        The credential handle is the same.

[karelz]

@krwq for the second, can you verify it manually / using other tools?

[karelz]

The first one sounds like a bug. Can you post a code snippet showing what you mean / what you observed?

[krwq]

@karelz - current version of the server works correctly with this bug because of the fallback I put in the code (see DecodeString in SniHelper). If you remove it one of the tests in SslStreamSniTest will start failing on Windows (the one with special characters) - see test data for this test:
https://github.com/dotnet/corefx/pull/28278/files#diff-dbaea2525913714cf555096c8af14a03R132

I believe client should doing IDN mapping but I'm currently not 100% sure this is a bug as I haven't seen the confirmation in the spec. Spec needs to be reread and checked for the words like "must".

[karelz]

Which test fails exactly? On SocketsHttpHandler or on PlatformHandler?
Let's chat about it in standup, I am confused what this is tracking and why.

[krwq]

@karelz this is a new test I'm adding in my PR (round-tripping hostName):
https://github.com/dotnet/corefx/pull/28278/files#diff-dbaea2525913714cf555096c8af14a03R21
with this data:
https://github.com/dotnet/corefx/pull/28278/files#diff-dbaea2525913714cf555096c8af14a03R132

I haven't seen any existing tests checking for that but also there was no good way to test it without both side support.

[krwq]

@karelz I've edited the original issue to make it clearer

[krwq]

@karelz, ok to move it to 2.1 with Post ZBB label?
EDIT: moving per offline conversation

[karelz]

Also, issues created after 3/28 automatically don't count against ZBB, so no worries.

2.1 goal: Assess impact from customer point of view - if usage of HttpClient in certain scenarios regressed between 2.0 and 2.1, then we need to address that. Users should not care about how HttpClient is implemented (as SocketsHttpHandler on 2.1 vs. WinHttpHandler/CurlHandler on 2.0), it should be transparent to them and all things should just work (except known differences like HTTP/2 support).

[krwq]

@karelz I've tested multiple scenarios and HttpClient seems to work correctly (does IDN mapping higher up the stack). I've tested behaviors on .NET Core, full .NET and also compared data send by the browser.

Only people using SslStream directly are affected by this issue. Moving to future.