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

Add TlsConnectionOptions to connection abstractions #42831

Closed
JamesNK opened this issue Jul 20, 2022 · 4 comments · Fixed by #42774
Closed

Add TlsConnectionOptions to connection abstractions #42831

JamesNK opened this issue Jul 20, 2022 · 4 comments · Fixed by #42774
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Milestone

Comments

@JamesNK
Copy link
Member

JamesNK commented Jul 20, 2022

Background and Motivation

Kestrel needs to pass TLS configuration to the QUIC transport. Neither project has a dependency on the other.

Communication is done through values added to IFeatureCollection. Rather than add a loose collection of types and delegates to the feature collection, I'd rather have a strongly typed options type that has the necessary configuration.

Also, part of TLS configuration is a callback that's called for each connection to resolve SslServerAuthenticationOptions. If the callback takes a context object, then new values can be added in the future.

Proposed API

Note: Microsoft.AspNetCore.Connections.Abstractions targets .NET Framework and .NET Standard. These types would only be present in .NET 7 it's the only target that supports SslServerAuthenticationOptions

namespace Microsoft.AspNetCore.Connections;

/// <summary>
/// Options used to configure a per connection callback for TLS configuration.
/// </summary>
public class TlsConnectionCallbackOptions
{
    /// <summary>
    /// The callback to invoke per connection. This property is required.
    /// </summary>
    public Func<TlsConnectionCallbackContext, ValueTask<SslServerAuthenticationOptions>> OnConnection { get; set; } = default!;

    /// <summary>
    /// Optional application state to flow to the <see cref="OnConnection"/> callback.
    /// </summary>
    public object? OnConnectionState { get; set; }

    /// <summary>
    /// Gets or sets a list of ALPN protocols.
    /// </summary>
    public List<SslApplicationProtocol> ApplicationProtocols { get; set; } = default!;
}

/// <summary>
/// Per connection state used to determine the TLS options.
/// </summary>
public class TlsConnectionCallbackContext
{
    /// <summary>
    /// Information from the Client Hello message.
    /// </summary>
    public SslClientHelloInfo ClientHelloInfo { get; set; }

    /// <summary>
    /// The information that was passed when registering the callback.
    /// </summary>
    public object? State { get; set; }

    /// <summary>
    /// The token to monitor for cancellation requests.
    /// </summary>
    public CancellationToken CancellationToken { get; set; }

    /// <summary>
    /// Information about an individual connection.
    /// </summary>
    public ConnectionContext Connection { get; set; } = default!;
}

Usage Examples

var features = new FeaturesCollection();

features.Set(new TlsConnectionCallbackOptions
{
    ApplicationProtocols = new List<SslApplicationProtocol> { SslApplicationProtocol.Http3 },
    OnConnection = context =>
    {
        return listenOptions.HttpsCallbackOptions.OnConnection(new TlsHandshakeCallbackContext
        {
            ClientHelloInfo = context.ClientHelloInfo,
            CancellationToken = context.CancellationToken,
            State = context.State,
            Connection = context.Connection,
        });
    },
    OnConnectionState = listenOptions.HttpsCallbackOptions.OnConnectionState,
});

Alternative Designs

These types are very similar to what is in Kestrel:

Risks

@JamesNK JamesNK added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Jul 20, 2022
@ghost
Copy link

ghost commented Jul 20, 2022

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@halter73
Copy link
Member

halter73 commented Jul 20, 2022

API review notes:

  • What about the location? Do we like putting this in the Microsoft.AspNetCore.Connections.Abstractions project?
    • This requires targeting multiple versions of .NET. This will only work with .NET 7.
    • This might work outside the shared framework if Kestrel doesn't consume it, so let's leave it where it is.
  • We should note in the doc comment to UseHttps() that the handshake timeout does not affect HTTP/3
    • Let's see if there's a way we can configure this for msquic
  • Do we have a ConnectionContext this early?
  • Why are we lying about nullability?
    • .NET standard support meant no init; or required.
    • These are always set in practice.
  • Should CancellationToken be on the context.
    • This is the same as TlsHandshakeCallbackContext
    • The prior art doesn't matter too much. Not that many people use TlsHandshakeCallbackContext.
    • Let's put the CancellationToken on the Func.

API Approved:

namespace Microsoft.AspNetCore.Connections;

/// <summary>
/// Options used to configure a per connection callback for TLS configuration.
/// </summary>
public class TlsConnectionCallbackOptions
{
    /// <summary>
    /// The callback to invoke per connection. This property is required.
    /// </summary>
    public Func<TlsConnectionCallbackContext, CancellationToken, ValueTask<SslServerAuthenticationOptions>> OnConnection { get; set; } = default!;

    /// <summary>
    /// Optional application state to flow to the <see cref="OnConnection"/> callback.
    /// </summary>
    public object? OnConnectionState { get; set; }

    /// <summary>
    /// Gets or sets a list of ALPN protocols.
    /// </summary>
    public List<SslApplicationProtocol> ApplicationProtocols { get; set; } = default!;
}

/// <summary>
/// Per connection state used to determine the TLS options.
/// </summary>
public class TlsConnectionCallbackContext
{
    /// <summary>
    /// Information from the Client Hello message.
    /// </summary>
    public SslClientHelloInfo ClientHelloInfo { get; set; }

    /// <summary>
    /// The information that was passed when registering the callback.
    /// </summary>
    public object? State { get; set; }

    /// <summary>
    /// Information about an individual connection.
    /// </summary>
    public BaseConnectionContext Connection { get; set; } = default!;
}

@halter73 halter73 added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Jul 20, 2022
@JamesNK
Copy link
Member Author

JamesNK commented Jul 21, 2022

I finished implementing this and I found the API needs to change slightly. A QUIC connection inherits from BaseConnectionContext and not ConnectionContext.

It make sense that QUIC connection doesn't inherit from ConnectionContext because that type adds an IDuplexPipe transport, which isn't valid for a multiplexed connection.

Change:

public class TlsConnectionCallbackContext
{
    /// <summary>
    /// Information about an individual connection.
    /// </summary>
-   public ConnectionContext Connection { get; set; } = default!;
+   public BaseConnectionContext Connection { get; set; } = default!;
}

BaseConnectionContext has The Good Stuff we want such as IP addresses. It's still useful to pass into the callback.

The existing UseHttps callback takes a ConnectionContext. We can't change that, but we still want to give the callback something so they can access IP addresses, so an internal adaptor wraps an inner BaseConnectionContext. Trying to access the pipe transport will error.

@halter73
Copy link
Member

Makes sense to me. I'm just going to edit the approved API to match.

@wtgodbe wtgodbe assigned JamesNK and unassigned JamesNK Jul 22, 2022
@adityamandaleeka adityamandaleeka added this to the 7.0-rc1 milestone Jul 25, 2022
@JamesNK JamesNK self-assigned this Jul 26, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Aug 27, 2022
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants