Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

don't send server_name when literal IP #81631

Merged
merged 3 commits into from
Feb 8, 2023
Merged

don't send server_name when literal IP #81631

merged 3 commits into from
Feb 8, 2023

Conversation

wfurt
Copy link
Member

@wfurt wfurt commented Feb 4, 2023

fixes #81590

I originally tried to use IPAddress.TryParse() But it allocates and drags in many dependencies.
Windows SChannel already do this so we could possibly skip it. But the check should be reasonably cheap so I left it in for all platforms.

HttpClient always sets it for IP addresses. I was hoping we could check Uri.HostNameType but by the time it hits the SslStream it is already passed as string and it is part of the connection pool processing. So left the fix-up only in SslStream.

We should probably do the same for Quic when we do #80508. (not sure if Quic allows to connect to empty string)

cc: @simonrozsival @nibanks

@wfurt wfurt added this to the 8.0.0 milestone Feb 4, 2023
@wfurt wfurt requested a review from rzikm February 4, 2023 07:07
@wfurt wfurt self-assigned this Feb 4, 2023
@ghost
Copy link

ghost commented Feb 4, 2023

Tagging subscribers to this area: @dotnet/ncl, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

fixes #81590

I originally tried to use IPAddress.TryParse() But it allocates and drags in many dependencies.
Windows SChannel already do this so we could possibly skip it. But the check should be reasonably cheap so I left it in for all platforms.

HttpClient always sets it for IP addresses. I was hoping we could check Uri.HostNameType but by the time it hits the SslStream it is already passed as string and it is part of the connection pool processing. So left the fix-up only in SslStream.

We should probably do the same for Quic when we do #80508. (not sure if Quic allows to connect to empty string)

cc: @simonrozsival @nibanks

Author: wfurt
Assignees: wfurt
Labels:

area-System.Net.Security

Milestone: 8.0.0

Copy link
Member

@rzikm rzikm left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@simonrozsival simonrozsival left a comment

Choose a reason for hiding this comment

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

LGTM! This partially fixes #79143 and will unblock System.Net.Http.Functional.Tests.SocketsHttpHandler_HttpClientHandlerTest.GetAsync_IPv6LinkLocalAddressUri_Success on Android 👍

@wfurt wfurt merged commit 1019ad8 into dotnet:main Feb 8, 2023
@wfurt wfurt deleted the sniIp branch February 8, 2023 20:14
@ghost ghost locked as resolved and limited conversation to collaborators Mar 11, 2023
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.

Inconsistent behavior in SslStream.AuthenticateAsClient across win/linux
3 participants