-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Add TargetHostName to QuicConnection #84976
Add TargetHostName to QuicConnection #84976
Conversation
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, 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. |
/azp run coreclr-libraries outerloop |
No pipelines are associated with this pull request. |
/azp list |
/azp run runtime-libraries-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
CI Failures are:
And more outerloop tests, all unrelated (System.Formats.Tar, System.Runtime.Numerics, ...) |
/azp run runtime-libraries-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
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.
Some comments, but looks good in general, thanks!
BTW, does this also fixes #55687 or is there still something else that needs to be done?
src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.SslConnectionOptions.cs
Outdated
Show resolved
Hide resolved
I think this should close it, MsQuic is actually encoding/decoding punycode for us. |
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.
LGTM, thank you!
/azp run runtime-libraries-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-libraries-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-libraries-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
Known test failures: Newly filed (unrelated to this PR) And many others (wasm and/or unrelated to SslStream/Quic) |
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.
LGTM
Fixes #80508, also ports #82934 and #81631 to System.Net.Quic to unify behavior.
I discovered that we have some inconsistency in TargetHostName on SslStream side, we will report the encoded TargetHost on client side TargetHostName property and in client cert selection callback, but decoded on server side. So I made changes to always store user-provided TargetHost and encode it only for the parts we need it. I also added a SslStream test for cert validation with IDN domain.
Closes #55687 (added tests which exercise IDN in S.N.Q).