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

Activate Quic Alpn Narrowing Test #91116

Merged
merged 6 commits into from
Sep 4, 2023

Conversation

liveans
Copy link
Member

@liveans liveans commented Aug 25, 2023

Fixes #86701

Looks like QuicListener.AcceptConnectionAsync doesn't throw AuthenticationException when the client's alpn is not present on the initial alpn list.

@ghost
Copy link

ghost commented Aug 25, 2023

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

Issue Details

Fixes #86701

Author: liveans
Assignees: -
Labels:

area-System.Net.Quic

Milestone: -

@liveans liveans marked this pull request as ready for review August 26, 2023 19:31
@liveans liveans requested a review from a team August 26, 2023 19:31
@wfurt
Copy link
Member

wfurt commented Aug 27, 2023

Fixes #86701

Looks like QuicListener.AcceptConnectionAsync doesn't throw AuthenticationException when the client's alpn is not present on the initial alpn list.

Should it? Perhaps we should get feedback from @ManickaP

@ManickaP
Copy link
Member

@liveans, AcceptConnectionAsync never returns in this case, doesn't it? NEW_CONNECTION event never comes for this connection, right? If so, then it makes sense, as MsQuic refuses the connection underneath and never gives it to us. This also happens with refused connections due to server overload.
In that case, this is fine as it is.

@liveans
Copy link
Member Author

liveans commented Aug 27, 2023

@liveans Ahmet Ibrahim Aksoy FTE, AcceptConnectionAsync never returns in this case, doesn't it? NEW_CONNECTION event never comes for this connection, right? If so, then it makes sense, as MsQuic refuses the connection underneath and never gives it to us. This also happens with refused connections due to server overload. In that case, this is fine as it is.

Yes, that's the case. AcceptConnectionAsync never returns in that case, but the listener will throw if the client's alpn is present on the initial list but not on the listener connection callback list, and this is expected.

Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

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

Small suggestion, otherwise LGTM, thanks!

@liveans
Copy link
Member Author

liveans commented Sep 4, 2023

/azp list

@azure-pipelines
Copy link

CI/CD Pipelines for this repository:

@liveans
Copy link
Member Author

liveans commented Sep 4, 2023

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liveans
Copy link
Member Author

liveans commented Sep 4, 2023

CI failures unrelated

@liveans liveans merged commit 7329c4b into dotnet:main Sep 4, 2023
151 of 176 checks passed
@karelz karelz added this to the 9.0.0 milestone Sep 14, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Oct 14, 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.

QUIC Listener ConnectionOptionsCallback ALPN Narrowing Down Failure with Wrong Error Code
4 participants