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 API for Kestrel named pipes transport #44612

Closed
JamesNK opened this issue Oct 18, 2022 · 6 comments
Closed

Add API for Kestrel named pipes transport #44612

JamesNK opened this issue Oct 18, 2022 · 6 comments
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

Comments

@JamesNK
Copy link
Member

JamesNK commented Oct 18, 2022

Background and Motivation

Public API for named pipes transport - #44426

The transport is still work-in-progress. This API issue reflects its ideal final API shape, not the PR's current state.

Proposed API

Feature to get the server stream of the name pipe connection. Used to access methods like GetImpersonatedUser and RunAs from the pipe. Similar to IConnectionSocketFeature.

namespace Microsoft.AspNetCore.Connections.Features;

/// <summary>
/// Provides access to the connection's underlying <see cref="NamedPipeServerStream"/>.
/// </summary>
public interface IConnectionNamedPipeFeature
{
    /// <summary>
    /// Gets the underlying <see cref="NamedPipeServerStream"/>.
    /// </summary>
    NamedPipeServerStream NamedPipe { get; }
}

Options type for named pipes transport. Similar to SocketTransportOptions.

namespace Microsoft.AspNetCore.Server.Kestrel.Transport.NamedPipes;

/// <summary>
/// Options for named pipe based transports.
/// </summary>
public sealed class NamedPipeTransportOptions
{
    /// <summary>
    /// The maximum length of the pending connection queue.
    /// </summary>
    /// <remarks>
    /// Defaults to 512 pending connections.
    /// </remarks>
    public int Backlog { get; set; } = 512;

    /// <summary>
    /// Gets or sets the maximum unconsumed incoming bytes the transport will buffer.
    /// <para>
    /// A value of <see langword="null"/> or 0 disables backpressure entirely allowing unlimited buffering.
    /// Unlimited server buffering is a security risk given untrusted clients.
    /// </para>
    /// </summary>
    /// <remarks>
    /// Defaults to 1 MiB.
    /// </remarks>
    public long? MaxReadBufferSize { get; set; } = 1024 * 1024;

    /// <summary>
    /// Gets or sets the maximum outgoing bytes the transport will buffer before applying write backpressure.
    /// <para>
    /// A value of <see langword="null"/> or 0 disables backpressure entirely allowing unlimited buffering.
    /// Unlimited server buffering is a security risk given untrusted clients.
    /// </para>
    /// </summary>
    /// <remarks>
    /// Defaults to 64 KiB.
    /// </remarks>
    public long? MaxWriteBufferSize { get; set; } = 64 * 1024;

    /// <summary>
    /// Gets or sets a value that indicates that the pipe can only be connected to by a client created by
    /// the same user account.
    /// <para>
    /// On Windows, a value of true verifies both the user account and elevation level.
    /// </para>
    /// </summary>
    /// <remarks>
    /// Defaults to true.
    /// </remarks>
    public bool CurrentUserOnly { get; set; } = true;

    /// <summary>
    /// Gets or sets the security information that determines the access control and audit security for pipes.
    /// </summary>
    public PipeSecurity? PipeSecurity { get; set; }
}

Extension methods to register named pipes services and/or customize options.

namespace Microsoft.AspNetCore.Hosting;

/// <summary>
/// <see cref="IWebHostBuilder" /> extension methods to configure the Named Pipes transport to be used by Kestrel.
/// </summary>
public static class WebHostBuilderNamedPipeExtensions
{
    /// <summary>
    /// Specify Named Pipes as the transport to be used by Kestrel.
    /// </summary>
    /// <param name="hostBuilder">
    /// The <see cref="IWebHostBuilder" /> to configure.
    /// </param>
    /// <returns>
    /// The <see cref="IWebHostBuilder" />.
    /// </returns>
    public static IWebHostBuilder UseNamedPipes(this IWebHostBuilder hostBuilder)
    {
        return hostBuilder.ConfigureServices(services =>
        {
            services.AddSingleton<IConnectionListenerFactory, NamedPipeTransportFactory>();
        });
    }

    /// <summary>
    /// Specify Named Pipes as the transport to be used by Kestrel.
    /// </summary>
    /// <param name="hostBuilder">
    /// The <see cref="IWebHostBuilder" /> to configure.
    /// </param>
    /// <param name="configureOptions">
    /// A callback to configure transport options.
    /// </param>
    /// <returns>
    /// The <see cref="IWebHostBuilder" />.
    /// </returns>
    public static IWebHostBuilder UseNamedPipes(this IWebHostBuilder hostBuilder, Action<NamedPipeTransportOptions> configureOptions)
    {
        return hostBuilder.UseNamedPipes().ConfigureServices(services =>
        {
            services.Configure(configureOptions);
        });
    }
}

New methods on KestrelServerOptions to register listener. There isn't a public NamedPipeEndPoint to pass to Listen(EndPoint) so these methods will be the only way to listen for a named pipe.

namespace Microsoft.AspNetCore.Server.Kestrel.Core;

public class KestrelServerOptions
{
    /// <summary>
    /// Bind to given named pipe.
    /// </summary>
    public void ListenNamedPipe(string pipeName);

    /// <summary>
    /// Bind to given named pipe.
    /// Specify callback to configure endpoint-specific settings.
    /// </summary>
    public void ListenNamedPipe(string pipeName, Action<ListenOptions> configure);
}

Usage Examples

var builder = WebApplication.CreateBuilder(args);

builder.WebHost.UseNamedPipes(); // note: may be registered by default in .NET 8
builder.WebHost.UseKestrel(o =>
{
    o.ListenNamedPipe("MyCoolPipe");
});

var app = builder.Build();

app.MapGet("/", (HttpContext context) =>
{
    var pipe = context.Features.Get<IConnectionNamedPipeFeature>().NamedPipe;
    return $"Hello {pipe.GetImpersonationUserName()}";
});

app.Run();

Alternative Designs

Risks

@JamesNK JamesNK added api-suggestion Early API idea and discussion, it is NOT ready for implementation api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Oct 18, 2022
@ghost
Copy link

ghost commented Oct 18, 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 Oct 27, 2022

API Review Notes:

  1. Can we remove the Backlog option until we prove it's necessary?
    • Yes.
  2. What about extending IServiceCollection instead of IWebHost?
    • Also seems fine.
    • AddNamedPipes? AddNamedPipesTransport? AddKestrelNamedPipes?
  3. In the future, we could write something similar to CreateBoundListenSocket to allow per-endpoint acl config.
namespace Microsoft.AspNetCore.Connections.Features;

+ public interface IConnectionNamedPipeFeature
+ {
+    NamedPipeServerStream NamedPipe { get; }
+ }

+ namespace Microsoft.AspNetCore.Server.Kestrel.Transport.NamedPipes;

+ public sealed class NamedPipeTransportOptions
+ {
+    public long? MaxReadBufferSize { get; set; } = 1024 * 1024;
+    public long? MaxWriteBufferSize { get; set; } = 64 * 1024;

+    public bool CurrentUserOnly { get; set; } = true;
+    public PipeSecurity? PipeSecurity { get; set; }
+ }

namespace Microsoft.Extensions.DependencyInjection;

+ public static class ServiceCollectionNamedPipeExtensions
+ {
+    public static IServiceCollection AddNamedPipesTransport(this IServiceCollection services);
+    public static IServiceCollection AddNamedPipesTransport(this IServiceCollection services, Action<NamedPipeTransportOptions> configureOptions);
+ }

namespace Microsoft.AspNetCore.Server.Kestrel.Core;

public class KestrelServerOptions
{
+    public void ListenNamedPipe(string pipeName);
+    public void ListenNamedPipe(string pipeName, Action<ListenOptions> configure);
}

@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 Oct 27, 2022
@adityamandaleeka adityamandaleeka added this to the .NET 8 Planning milestone Oct 31, 2022
@halter73
Copy link
Member

Feedback from @JamesNK

I think the service wire up methods could go back to the original proposal:

namespace Microsoft.AspNetCore.Hosting;

public static class WebHostBuilderNamedPipeExtensions
{
+   public static IWebHostBuilder UseNamedPipes(this IWebHostBuilder hostBuilder);
+   public static IWebHostBuilder UseNamedPipes(this IWebHostBuilder hostBuilder, Action<NamedPipeTransportOptions> configureOptions);
}

Reasons:
When using WebApplication: builder.WebHost.XXX() vs builder.Services.XXX() seems the same to me.
This method will be used very infrequently. Extending web host builder prevents it from polluting IServiceCollection.
Consistent with existing UseSockets, UseQuic.
My inclination is consistency, but I get the argument that it could be AddNamedPipesTransport on IWebHostBuilder since Kestrel is adding support for multiple transports.

And.

I found a couple of extra APIs are needed while implementing the named pipe transport. I want to do an email review for these remaining changes.

KestrelServerOptions.ListenNamedPipe method needs to endpoint type to give to the transport. The types are in different assemblies, and connection abstractions is shared between them. That means a public endpoint type for named pipes is needed:

namespace Microsoft.AspNetCore.Connections;

+ public sealed class NamedPipeEndPoint : EndPoint
+ {
+    // A server name of "." refers to the local machine.
+    public NamedPipeEndPoint(string pipeName) : this(pipeName, serverName: ".");
+    public NamedPipeEndPoint(string pipeName, string serverName);
+    public string ServerName { get; }
+    public string PipeName { get; }
+ }

There is prior art for doing this. A a couple of other EndPoint implementations, e.g. FileHandleEndPoint, are already in connection abstractions.

The server name isn't used by Kestrel because it hosts named pipes, which is always on the current machine. I left server name as an option in case someone wants to use NamedPipeEndPoint in a client, which can connect to named pipes on a different machine. It could be removed.

The other new API is a property on ListenOptions that reports the bound pipe name. There are similar properties for bound IP addresses, Unix domain socket paths, etc, on ListenOptions.

public class ListenOptions
{
+    public string? PipeName { get; }
}

This property isn't necessary, someone could access the endpoint via a property and get the pipe name from it, but it is consistent with what is already on the options type.

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

ghost commented Nov 10, 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 Nov 10, 2022

API review notes:

  • Can we read a named pipe endpoint from config like a unix:// domain socket?
    • Not yet.
    • What should the scheme be? net.pipe://? Is this WCF only? Might bet specific to .NET?
    • Can you configure https from the URL?
  • What do we think about UseNamedPipes IWebHostBuilder extension method vs AddNamedPipesTransport IServiceCollection extension method? We like it for consistency. IWebHostBuilder isn't going anywhere.
  • Is NamedPipeEndpoint adequately bidirectional? Yes. There's a second constructor that allows a remote server.

API Approved! Again!

namespace Microsoft.AspNetCore.Connections.Features;

+ public interface IConnectionNamedPipeFeature
+ {
+    NamedPipeServerStream NamedPipe { get; }
+ }

+ namespace Microsoft.AspNetCore.Server.Kestrel.Transport.NamedPipes;

+ public sealed class NamedPipeTransportOptions
+ {
+    public long? MaxReadBufferSize { get; set; } = 1024 * 1024;
+    public long? MaxWriteBufferSize { get; set; } = 64 * 1024;

+    public bool CurrentUserOnly { get; set; } = true;
+    public PipeSecurity? PipeSecurity { get; set; }
+ }

namespace Microsoft.AspNetCore.Hosting;

public static class WebHostBuilderNamedPipeExtensions
{
+   public static IWebHostBuilder UseNamedPipes(this IWebHostBuilder hostBuilder);
+   public static IWebHostBuilder UseNamedPipes(this IWebHostBuilder hostBuilder, Action<NamedPipeTransportOptions> configureOptions);
}

namespace Microsoft.AspNetCore.Server.Kestrel.Core;

public class KestrelServerOptions
{
+    public void ListenNamedPipe(string pipeName);
+    public void ListenNamedPipe(string pipeName, Action<ListenOptions> configure);
}

public class ListenOptions
{
+    public string? PipeName { get; }
}

// Microsoft.AspNetCore.Connections.Abstractions.dll
namespace Microsoft.AspNetCore.Connections;

+ public sealed class NamedPipeEndPoint : EndPoint
+ {
+    // A server name of "." refers to the local machine.
+    public NamedPipeEndPoint(string pipeName) : this(pipeName, serverName: ".");
+    public NamedPipeEndPoint(string pipeName, string serverName);
+    public string ServerName { get; }
+    public string PipeName { get; }
+ }

@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 Nov 10, 2022
@adityamandaleeka
Copy link
Member

@JamesNK can this be closed?

@JamesNK JamesNK closed this as completed Mar 29, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Apr 29, 2023
@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

No branches or pull requests

4 participants