RequireLocalPort and related extensions for IEndpointConventionBuilder

[hacst]

Background and Motivation

This is basically a continuation of the discussion in #46057 which got closed due to missing author feedback.

Currently the asp.net core API surface offers no straight-forward way to restrict access to an endpoint to a specific port or host. In many locations on the net, including Microsoft documentation like Health-Checks patterns like

app.MapHealthChecks("/healthz")
    .RequireHost("*:5001");

are suggested to enforce a port restriction. However this is an unsafe way to implement such a restriction as host matching is only performed based on the often user controllable Host header of the request. This is especially dangerous as the restriction appears to work when using a benign client like a browser.

https://learn.microsoft.com/en-us/aspnet/core/fundamentals/routing?view=aspnetcore-7.0#host-matching-in-routes-with-requirehost and the extension method documentation in https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.builder.routingendpointconventionbuilderextensions.requirehost?view=aspnetcore-7.0 also do not make it clear that this relies on the potentially user controlled Host header field.

To make it easy for users to get correct and safe behavior when trying to restrict access to a specific port, having extension methods to match on HttpContext.Connection.LocalPort would be helpful. A related though probably rarer and more complicated problem would be to want to require connections from a specific ip (or interface) if multiple are bound. The basic ip restriction version could match on HttpContext.Connection.LocalIpAddress . To get full control the ability to match exactly what the server actually listens on (e.g. could even be a unix domain socket) would be needed.

I reported the specific health check documentation issue in https://github.com/dotnet/AspNetCore.Docs/issues/29399 and was asked to create a more general issue here.

Proposed API

As suggested in #46057 in addition to improving the documentation on RequireHost, there could be a RequireLocalPort extension method for IEndpointConventionBuilder.

Additionally a corresponding RequireLocalIpAddress could be added.

namespace Microsoft.AspNetCore.Builder;

public static class RoutingEndpointConventionBuilderExtensions
{
+    public static TBuilder RequireLocalPort<TBuilder>(this TBuilder builder, params int[] ports) where TBuilder : IEndpointConventionBuilder { ... };
+    public static TBuilder RequireLocalIpAddress<TBuilder>(this TBuilder builder, params string[] ips) where TBuilder : IEndpointConventionBuilder { ... };
}

To give full control from where a connection is coming from, additional extension methods matching the corresponding listen endpoint possibilities could be added. However I do not think I have enough framework knowledge to propose good and implementable api for these. Maybe something in the direction of:

namespace Microsoft.AspNetCore.Builder;

public static class RoutingEndpointConventionBuilderExtensions
{
+    public static TBuilder RequireNetEndPoint<TBuilder>(this TBuilder builder, params System.Net.EndPoint[] socketEndpoint) where TBuilder : IEndpointConventionBuilder { ... };
}

Usage Examples

app.MapGet( "/only5001", () =>"Only reachable on port 5001")
  .RequireLocalPort(5001);

app.MapGet( "/onlyloopback", () => "Only reachable through loopback")
  .RequireLocalIpAddress("127.0.0.1", "[::1]");

UnixDomainSocketEndPoint domainSocket = ...;
app.MapGet( "/onlydomainsocket", () => "Only reachable on domain socket")
  .RequireNetEndPoint(domainSocket);

Alternative Designs

• To simplify usage something like a RequireLocalHost analog to RequireHost could be introduced that understands things like localhost:1234, *:80, 127.0.0.1.:*. However this is easy to confuse with RequireHost and due to the flexible interface it is not so simple to answer what should happen if it were given a DNS name or any other current/future options RequireHost might support.

• RequireLocalIpAddress could take an actual IPAddress object instead of a string. In practice I think this would just lead to the vast majority of users having to add a IPAddress.Parse into the call. Of course both overloads could be offered.

• The methods could be abbreviated to RequirePort and RequireIpAddress. This makes it harder to understand that this matches a specific part of the HttpContext.Connection and if "remote" were added later could be slightly harder to understand. On the other hand the "Local" part might confuse some.

cc: @guardrex https://github.com/dotnet/AspNetCore.Docs/issues/29399

[Tratcher]

• RequireLocalIpAddress could take an actual IPAddress object instead of a string. In practice I think this would just lead to the vast majority of users having to add a IPAddress.Parse into the call. Of course both overloads could be offered.

Yes, include both overloads. I expect static instances like IPAddress.Loopback to be commonly used.

Since the APIs take lists, the names should be plural.

[hacst]

Yes, include both overloads. I expect static instances like IPAddress.Loopback to be commonly used.

Makes sense. Do I just edit the proposal for that or what is the process?

Since the APIs take lists, the names should be plural.

I oriented myself after the RequireHost which is singular but takes a variable amount of arguments. I assumed the idea was that most callers would only pass one argument and hence the singular is preferrable.

[davidfowl]

RequireLocalPort -👍🏾 RequireLocalIpAddress - Why isn't this hosts, why does it need to be ips? RequireNetEndPoint - How would this work?

[hacst]

RequireLocalIpAddress - Why isn't this hosts, why does it need to be ips?

I did consider it. However as at the "receiving a normal network connection" level the host is not a thing, something the user might expect to be able to pass for a host like "myhost.com" would have to be turned into a set of IPs in a predictable way to compare the connection against. Given how name resolution like DNS works and is used (e.g. traffic management) this felt to brittle to me for this API. Is there some precedent in .NET where "host" is defined and successfully used in such tasks that could solve this?

Limiting it to IPs otherwise would of course also work. Also being able to specify "localhost" and have it bind ipv4 and ipv6 loopback with one call like the server does would be useful. E.g.

 x.RequireLocalHost("24.24.24.24:1234");
 y.RequireLocalHost("*:3456");
 z.RequireLocalHost("localhost");

(note: I do not like that name for the function)

The IP based interface on its own seemed well specified and useful as an addition to .RequireLocalPort even if it would overlap with a more "host" like variant down the line so I thought it would be the better option to propose.

RequireNetEndPoint - How would this work?

As mentioned this is where my framework knowledge is insufficient to make a good proposal or judge how/whether it would fit into the architecture. The goal would be to be able to restrict an endpoint to whatever the underlying server is actually bound/listens to in the most precise way. In the case of Kestrel it looked like that might be a number of Net.EndPoints though I might very well be mistaken. For things like unix domain sockets this would probably require being able to query the actual EndPoint in some way from the server to be able to pass it, as what is under a specific path might've changed in the meantime. Whether this can then be checked where current host matching happens in a portable way I don't know.

[davidfowl]

This component works by looking at data from the incoming HttpContext. When you specify RequireHost it looks at the host header. There is no binding information about the endpoint today that flows on the HttpContext. It seems like this feature is trying to filter based on what the listener accepted the connection and subsequently requests (or I'm missing something). IF that is the case, then we should split this into two issues.

1. Syntax sugar on top of RequireHost (RequireLocalPort).
2. A new feature that requires flowing the binding information into the request so that route matching and filtering can be done on it.

[hacst]

Sounds reasonable. I wasn't aware of the implementation complications here. I have no problem dropping/splitting out the binding part. I added it mostly for completion. Do I just remove that part from the proposal or keep it as is while discussing?

[davidfowl]

Lets wait for @Tratcher and others to chime in. I just took a look at what we would need to do to make this possible but I think it requires adding quite a more API than proposed.

[Tratcher]

You don't need the binding information, HttpContext.Connection.LocalIPAddress/Port should be enough.

[davidfowl]

Is that currently set for unix domain sockets? Is the assumption that we are filtering on the Listen endpoint or the connection endpoint (they aren't the same right?)

[Tratcher]

No, I wouldn't expect this to apply to UDS. These would filter out all UDS connections because they lack the required fields.

The port will be the same for a binding and a connection and I think that's the predominant request here.

IPs are a little fuzzier. You can bind to * and connect to any of the local IPs. You can bind to localhost and connect to IPv4 or 6 loopback. But if you really wanted to filter a route by a specific IP, there's no reason you couldn't, I just expect it to be less common than filtering by port.

[davidfowl]

@Tratcher I'm not sure I understand. I think this works for the host name, I don't see how it works for IP or for any arbitrary transport kestrel could plug in (UDS, Memory, etc).

The idea I had was that we would expose another field that describes the listen endpoint for the current request and then this could be built on top of that. It also enables scenarios that are hard to make work today (have endpoints that only work for unix domain sockets vs not).

[Tratcher]

Here's another health check implementation that could benefit from this: https://github.com/dotnet/aspnetcore/blob/ff789e353981d1e4f812c754d1e1cd05a3da9724/src/Middleware/HealthChecks/src/Builder/HealthCheckApplicationBuilderExtensions.cs#L210-L229

[hacst]

Any way I can help to advance this?

[schmitch]

it would be great if AddEndpointFilter would be able to say, skip this middleware instead of only allowing to short-ciruit the endpoint, than this feature could be emulated via AddEndpointFilter.