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

throw PNSE for unsupported SSL options in Quic. #55877

Merged
merged 3 commits into from
Jul 21, 2021
Merged

Conversation

wfurt
Copy link
Member

@wfurt wfurt commented Jul 17, 2021

throw for few options we do not support to make it obvious

fixes #55759

@wfurt wfurt requested a review from a team July 17, 2021 23:57
@wfurt wfurt self-assigned this Jul 17, 2021
@ghost
Copy link

ghost commented Jul 17, 2021

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

Issue Details

throw for few options we do not support to make it obvious

fixes #55759

Author: wfurt
Assignees: wfurt
Labels:

area-System.Net.Quic

Milestone: -

@am11
Copy link
Member

am11 commented Jul 18, 2021

throw PNSP for unsupported SSL options in Quic.

PNSP -> PNSE?

@wfurt wfurt changed the title throw PNSP for unsupported SSL options in Quic. throw PNSE for unsupported SSL options in Quic. Jul 18, 2021
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.

Where would the check be for #55468?

@wfurt
Copy link
Member Author

wfurt commented Jul 19, 2021

Where would the check be for #55468?

I don't understand the comment @ManickaP. I assume #55468 would go in so I did not add check ServerCertificateSelectionCallback just to remove it later. If you had something else in mind let me know.

@ManickaP
Copy link
Member

You've added the validation into Create methods, which is great and makes sense here. In that PR you're adding a new Create method, which is not called when the listener is initialized, but later in the game. So I'm asking, how will the validation look like in that case? When and where will it happen once that PR is in?

@wfurt
Copy link
Member Author

wfurt commented Jul 19, 2021

ok, that make sense. I will update #55468 as well since I'm going to probably touch it anyway. We may split pre-validation to separate method but I'm not sure I want to do that right now. ConfigurationOpenDelegate may still fail so we should figure out how to better surface exceptions.

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.

LGTM, thanks. Looking forward for the SNI PR solution.

@wfurt
Copy link
Member Author

wfurt commented Jul 21, 2021

failing test fixed by #56047.

@wfurt wfurt merged commit b2107c5 into dotnet:main Jul 21, 2021
@wfurt wfurt deleted the pnsp_55759 branch July 21, 2021 03:12
karelz added a commit that referenced this pull request Jul 21, 2021
wfurt pushed a commit that referenced this pull request Jul 21, 2021
@karelz karelz added this to the 6.0.0 milestone Jul 22, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Aug 21, 2021
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] Throw PNSE for unsupported SslOptions (client and server)
4 participants