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

QUIC: Validate SslServerAuthenticationOptions set on QuicListenerOptions #49423

Closed
JamesNK opened this issue Mar 10, 2021 · 20 comments · Fixed by #86659
Closed

QUIC: Validate SslServerAuthenticationOptions set on QuicListenerOptions #49423

JamesNK opened this issue Mar 10, 2021 · 20 comments · Fixed by #86659
Labels
area-System.Net.Quic test-enhancement Improvements of test source code
Milestone

Comments

@JamesNK
Copy link
Member

JamesNK commented Mar 10, 2021

QuicListenerOptions has SslServerAuthenticationOptions property.

public SslServerAuthenticationOptions? ServerAuthenticationOptions { get; set; }

It looks like code that uses this property assumes that SslServerAuthenticationOptions.ServerCertificate is set.

  • Need to validate that it is set.
  • Need to consider what happens when other options are set like a certificate selector callback.
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Mar 10, 2021
@ghost
Copy link

ghost commented Mar 10, 2021

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

Issue Details

QuicListenerOptions has SslServerAuthenticationOptions property.

public SslServerAuthenticationOptions? ServerAuthenticationOptions { get; set; }

It looks like code that uses this property assumes that SslServerAuthenticationOptions.ServerCertificate is set. * Need to validate that it is set.

  • Need to consider what happens when other options are set like a certificate selector callback.
Author: JamesNK
Assignees: -
Labels:

area-System.Net.Quic

Milestone: -

@JamesNK JamesNK changed the title HTTP/3: Validate SslServerAuthenticationOptions set on QuicListenerOptions QUIC: Validate SslServerAuthenticationOptions set on QuicListenerOptions Mar 10, 2021
@ManickaP ManickaP removed the untriaged New issue has not been triaged by the area owner label Mar 11, 2021
@ManickaP ManickaP added this to the 6.0.0 milestone Mar 11, 2021
@ManickaP
Copy link
Member

Triage: we should consider making it non-nullable if it needs to be set. Also decide what to do about the other properties, which is probably covered in #32071.

@ManickaP ManickaP modified the milestones: 6.0.0, 7.0.0 Jul 15, 2021
@ManickaP
Copy link
Member

Triage: this is nice to have, moving to 7.0

@JamesNK
Copy link
Member Author

JamesNK commented Jul 29, 2021

@Tratcher

@wfurt
Copy link
Member

wfurt commented Jul 29, 2021

Note that MsQuicListener needs ALPN to event start. That may be fixed in future but that is current constraint.

@karelz karelz added the bug label Nov 16, 2021
@ManickaP ManickaP self-assigned this Feb 8, 2022
@ManickaP
Copy link
Member

ManickaP commented Feb 8, 2022

We need the SslOptions, they shouldn't be nullable (API shape to discuss): #49587 (comment)
We do validate/handle most of the options on the class, the missing are listed here: #32071 (comment)

@karelz
Copy link
Member

karelz commented Feb 8, 2022

Triage: Current behavior is enforced to allow multiple protocols to be on same UDP port (HTTP and SMB on UDP 443). cc @nibanks
@JamesNK @Tratcher are you able to provide the ALPN upfront?

@JamesNK
Copy link
Member Author

JamesNK commented Feb 8, 2022

I believe Kestrel will always use SslApplicationProtocol.Http3 as the ALPN.

Kestrel doesn't offer a way to customize the ALPN for HTTP/1.1 or HTTP/2. Right now I don't see that changing for HTTP/3.

@Tratcher
Copy link
Member

Tratcher commented Feb 8, 2022

I don't expect custom ALPN per SNI to be an issue until we get to HTTP/4 (maybe next year 😆).

@wfurt
Copy link
Member

wfurt commented Feb 8, 2022

We have little bit more chat about this with @nibanks. The reason why msquic want ALPN upfront is to be able share the port with independent applications. Something that does not work with TCP/TLS but it is possible with QUICK.

maybe the flow would be to get everything Kestrel may support upfront and then select specific HTTP version in the callback ...? (H3 only one for now)

@JamesNK
Copy link
Member Author

JamesNK commented Feb 8, 2022

Chris, have you ever seen people use SNI to configure a non-standard ALPN? Would Kestrel even allow that?

Hypothetical future scenario:

  • A Kestrel port is configured to support HTTP/3 and future HTTP/4. Kestrel knows upfront that the only ALPN's it cares about for the port are SslApplicationProtocol.Http3 and a future SslApplicationProtocol.Http4.
  • Port is also configured to use ServerOptionsSelectionCallback to customize SslServerAuthenticationOptions based on server name. Based on the server name the callback returns auth options that have HTTP/3 and/or HTTP/4 ALPN. If a different ALPN is configured in the callback then we throw an error saying an unexpected ALPN was provided.

Workable?

@Tratcher
Copy link
Member

Tratcher commented Feb 8, 2022

I've seen some apps that have conditionally enabled/disabled HTTP/2 per SNI by customizing the ALPN. That works fine with Kestrel. You can only use protocols kestrel knows about though.

Hypothetical future scenario:

  • A Kestrel port is configured to support HTTP/3 and future HTTP/4. Kestrel knows upfront that the only ALPN's it cares about for the port are SslApplicationProtocol.Http3 and a future SslApplicationProtocol.Http4.
  • Port is also configured to use ServerOptionsSelectionCallback to customize SslServerAuthenticationOptions based on server name. Based on the server name the callback returns auth options that have HTTP/3 and/or HTTP/4 ALPN. If a different ALPN is configured in the callback then we throw an error saying an unexpected ALPN was provided.

Maybe? It's worth a try.

@ManickaP
Copy link
Member

@nibanks when we open listener we need to provide list of ALPN to listen for, but we also set ALPN when we call ConnectionSetConfiguration when we accept the connection. What would happen if the ALPN list here changed? Let's say narrowed down?

@nibanks
Copy link

nibanks commented Feb 15, 2022

I believe any change would be ignored. The ALPN has already been negotiated by the time you get the Listener callback.

@wfurt
Copy link
Member

wfurt commented Feb 15, 2022

We still should be able to set the certificate, right @nibanks? (e.g. the handshake is not fully finished)

@nibanks
Copy link

nibanks commented Feb 15, 2022

Yes, the call to ConnectionSetConfiguration should succeed, unless the TLS layer does some extra validation and rejects it.

@ManickaP
Copy link
Member

ManickaP commented Feb 22, 2022

Triage: open issue in msquic to be able to narrow down the ALPN selection when connection arrives. But we need to provide the full supported list when we start the listener.

msquic issue: microsoft/msquic#2418

@Tratcher
Copy link
Member

To closely match the SslStream API, the second stage ALPN selection would still be a prioritized list rather than a single entry. S.N.Q or MsQuic would be responsible for the final selection.

@karelz
Copy link
Member

karelz commented Jun 13, 2022

Triage: This is about future proofing -- once we have more than 1 ALPN for HTTP over QUIC. Currently there is only one "h3".
Direct QUIC users will have to set ALPN upfront.

@ManickaP
Copy link
Member

Triage: msquic issue has been fixed, we should add a test for this and call it done.

@ManickaP ManickaP modified the milestones: Future, 8.0.0 Jan 30, 2023
@ManickaP ManickaP added test-enhancement Improvements of test source code and removed bug labels Jan 30, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 23, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jun 22, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jul 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Quic test-enhancement Improvements of test source code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants