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

fix SNI handling in quic #55468

Merged
merged 8 commits into from
Jul 23, 2021
Merged

fix SNI handling in quic #55468

merged 8 commits into from
Jul 23, 2021

Conversation

wfurt
Copy link
Member

@wfurt wfurt commented Jul 11, 2021

This is attempt to close functional gap between SslStream and QUIC with focus on server part.
This PR has three big chunks:

  1. HTTP/3 requires client to send SNI but we did not send any.
    Current code would marshal IP address as string but we would never pass in the target server name.
    To fix that I added QUIC_PARAM_CONN.REMOTE_ADDRESS. When remote address is set, MsQuic only saves string passed to ConnectionStart as SNI and does not tries to connect to it.
    I did not see way how to set SNI separately. Perhaps @nibanks can comment but I don't know how to set SNI to separate value for DNS endpoint e.g. if the SNI does not match host one connects to.

  2. Adding support for ServerCertificateSelectionCallback and add ability the select different certificates on single listener.
    This is somewhat straight forward. I decide to delay creation of SafeMsQuicConfigurationHandle. Since the callback is synchronous we call it when we get the SNI from client. We will create multiple configurations and there is probably opportunity for caching. SafeHandle should do right thing and release underlying memory when closed.

When callback fails or returns null for particular SNI we will abort whole Listener and that feels unpleasant.
However I feel fixing the error behavior is not in scope of this PR.

fixes #55421

@wfurt wfurt requested review from stephentoub and a team July 11, 2021 04:26
@wfurt wfurt self-assigned this Jul 11, 2021
@dotnet-issue-labeler
Copy link

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.

@ghost
Copy link

ghost commented Jul 11, 2021

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

Issue Details

This is attempt to close functional gap between SslStream and QUIC with focus on server part.
This PR has three big chunks:

  1. HTTP/3 requires client to send SNI but we did not send any.
    Current code would marshal IP address as string but we would never pass in the target server name.
    To fix that I added QUIC_PARAM_CONN.REMOTE_ADDRESS. When remote address is set, MsQuic only saves string passed to ConnectionStart as SNI and does not tries to connect to it.
    I did not see way how to set SNI separately. Perhaps @nibanks can comment but I don't know how to set SNI to separate value for DNS endpoint e.g. if the SNI does not match host one connects to.

  2. Adding support for ServerCertificateSelectionCallback and add ability the select different certificates on single listener.
    This is somewhat straight forward. I decide to delay creation of SafeMsQuicConfigurationHandle. Since the callback is synchronous we call it when we get the SNI from client. We will create multiple configurations and there is probably opportunity for caching. SafeHandle should do right thing and release underlying memory when closed.

When callback fails or returns null for particular SNI we will abort whole Listener and that feels unpleasant.
However I feel fixing the error behavior is not in scope of this PR.

fixes #55421

  1. add support for ServerOptionsSelectionCallback
    This one is somewhat similar to the one above. There are additional complications:
  • while MsQuic is happy to delay certificate it takes ALPN in ListenerStart. So we need part of the ServerOptions before staring listener. It would be great if we can defer ALPN processing until we have certificate as well. Perhaps @nibanks can comment on this.
  • ServerOptionsSelectionCallback is asynchronous. That gives option to fetch certificates (and other setting) asynchronously. For now, the callback will just await blocking MsQuic's thread. We should be eventually able to return Pending and restore the connection when ready. (not done here)
  • For API shape. For now I added this to ListenerOptions so one can specify the callback. SslStream. also takes pass-through object and cancellation token. I added first and token is TBD. SslStream has separate *AuthenticateAs() but I was not sure this is appropriate for Listener. Since Quic is private the Option route should be mostly sufficient this should provide similar functionality.

fixes #49587

Author: wfurt
Assignees: wfurt
Labels:

area-System.Net.Quic

Milestone: -

@geoffkizer
Copy link
Contributor

@nibanks Can you take a look at the issues raised above in the original description here?

@nibanks
Copy link

nibanks commented Jul 12, 2021

It would be great if we can defer ALPN processing until we have certificate as well. Perhaps @nibanks can comment on this.

We cannot do that. MsQuic must have the ALPNs so that it can multiplex parallel, independent listeners for the different protocols on the same local port (e.g. HTTP and SMB).

@wfurt
Copy link
Member Author

wfurt commented Jul 12, 2021

Could you simply start single listener in this case? It seems unlikely that somebody would run SMB on same port as HTTP server. Running multiple HTTP servers with different policies seems quite common. That can be Kestrel or YARP or any multi-tenant deployment.

if (state.AuthenticationOptions.ServerCertificateSelectionCallback != null)
{
// ServerCertificateSelectionCallback is synchronous. We will call it as needed when building configuration
connectionConfiguration = SafeMsQuicConfigurationHandle.Create(state.ConnectionOptions, state.AuthenticationOptions, targetHost);
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably shouldn't be calling the user's callback on the msquic thread, right?

If we need to do this for now, then that's fine; but we should at least file an issue on this.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok. We already do that for validation. We can change them both but we would end up with quite a bit more complicated code IMHO. Aside from handling exceptions what are the main drawbacks of running from the event handler?

@karelz
Copy link
Member

karelz commented Jul 15, 2021

@geoffkizer this is ready for another round of code review ...

try
{
uint status = MsQuicApi.Api.ConnectionStartDelegate(
status = MsQuicApi.Api.ConnectionStartDelegate(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
status = MsQuicApi.Api.ConnectionStartDelegate(
uint status = MsQuicApi.Api.ConnectionStartDelegate(

It shouldn't clash if you put the declaration above next to the SetParamDelegate as well. They should be within different scopes.

@hoyosjs
Copy link
Member

hoyosjs commented Jul 21, 2021

Please ignore the runtime-nikola-test-reenable runs - I made a slight pipeline authoring mistake.

@wfurt
Copy link
Member Author

wfurt commented Jul 21, 2021

Aside from direct feedback I did following updates:

  • move some variables from MsQuicConnection to State so we do not need to capture connection for certificate validation.
  • Add initial support to surface validation to the caller so he/she get specific failures instead of anonymous reset.
  • Make validation failures AuthenticationException to match SslStream.

handle processing is still messed up. I will address that as separate PR.

@ManickaP ManickaP merged commit 5399ee5 into dotnet:main Jul 23, 2021
@wfurt wfurt deleted the sniQuic branch July 23, 2021 17:26
@karelz karelz added this to the 6.0.0 milestone Jul 29, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Aug 28, 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] Server side certificate selection via ServerCertificateSelectionCallback should work
6 participants