Skip to content

Conversation

@wfurt
Copy link
Member

@wfurt wfurt commented Jun 21, 2020

This exposes internal property used with SNI. This will return string.Empty if not available to be consistent with ServerCertificateSelectionCallback.

Strangely, when empty name is provided, we fabricate random name instead of skipping SNI extension. This is probably not common case so I made the property to return the fabricated name instead of empty value from SslClientAuthenticationOptions. On server, this gets name requested by client. Since the behavior above, it is not easy to construct test without SNI.

related to #37933
fixes #27619

@wfurt wfurt requested review from a team and bartonjs June 21, 2020 20:08
@wfurt wfurt self-assigned this Jun 21, 2020
@ghost
Copy link

ghost commented Jun 21, 2020

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

@Dotnet-GitSync-Bot
Copy link
Collaborator

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@stephentoub
Copy link
Member

Strangely, when empty name is provided, we fabricate random name instead of skipping SNI extension

What kind of name do we fabricate? Can you provide an example?

@wfurt
Copy link
Member Author

wfurt commented Jun 22, 2020

Strangely, when empty name is provided, we fabricate random name instead of skipping SNI extension

What kind of name do we fabricate? Can you provide an example?

Something like ?124 which looks lame to me but I don't know the history of it.

if (_sslAuthenticationOptions.TargetHost!.Length == 0)
{
_sslAuthenticationOptions.TargetHost = "?" + Interlocked.Increment(ref s_uniqueNameInteger).ToString(NumberFormatInfo.InvariantInfo);
}

@stephentoub
Copy link
Member

Something like ?124 which looks lame to me but I don't know the history of it.

That does look really strange. This was inherited from .NET Framework:
https://referencesource.microsoft.com/#System/net/System/Net/SecureProtocols/_SslState.cs,172
@davidsh, do you know what the purpose of this is?

@wfurt
Copy link
Member Author

wfurt commented Jun 22, 2020

When no name is provided then it is harder for ssl client to verify serve's name.
But that can be handled in the callback. I did verify that the fabricated name get sent on wire as part of the ClientHello.
I did not want to make functional change in this PR but I can perhaps open new issue for investigation. I'm wondering if this was/is some limitatuon of Windows Schannel API.

Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

We can follow-up on the strange constructed names subsequently. Is there an issue for that?

@wfurt
Copy link
Member Author

wfurt commented Jun 24, 2020

We can follow-up on the strange constructed names subsequently. Is there an issue for that?

#38356

@wfurt wfurt merged commit 5f19ea1 into dotnet:master Jul 1, 2020
@wfurt wfurt deleted the targetHostName_27619 branch July 1, 2020 20:45
@karelz karelz added this to the 5.0.0 milestone Aug 18, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add SslStream.ServerName and other properties from SslClientAuthenticationOptions

4 participants