Enable SocketsHttpHandler.ConnectCallback to negotiate TLS

[brporter]

AB#1219925

Description

When using the new ConnectCallback feature, returning an SslStream results in errors related to TLS negotiation. HttpConnectionPool expects to be in charge of negotiating SSL over the returned stream, but design eliminates lots of possible transport-level opportunities for people who want or need to control or manage transport processing.

Here is an example implementation of ConnectCallback that will fail as a result of this issue.

            var retVal = new HttpMessageInvoker(
                new SocketsHttpHandler()
                {
                    ConnectCallback = async (ctx, cancellationToken) =>
                    {
                        var socket = new Socket(SocketType.Stream, ProtocolType.Tcp);
                        await socket.ConnectAsync(ctx.DnsEndPoint);

                        var networkStream = new NetworkStream(socket);
                        var sslStream = new SslStream(networkStream);

                        await sslStream.AuthenticateAsClientAsync(ctx.RequestMessage.RequestUri.Host);

                        return sslStream;
                    }
                }
            );

Ideally, HttpConnectionPool would manage negotiating TLS for me if I returned a Stream implementation, but if I returned an SslStream, it ought to defer negotiation to me.

Configuration

Latest bits from release/5.0-rc2, not arch or platform specific.

Regression?

Not a regression.

Other information

Being able to return an SslStream (or other stream types) is important because it enables the ConnectCallback to provide a facility for transport-level optimizations like stream multiplexing (imagine a stream implementation that's built atop a series of TCP sockets for compatibility reasons, but actually parallelizes writes over multiple underlying TCP connections), TLS pre-negotiation (TLS negotiation for small payloads is expensive, perhaps the implementor of the ConnectCallback would like to maintain a pool of pre-negotiated TLS sessions for specific destinations) and telemetry (streams returned by ConnectCallback can't effectively have their contents intercepted by other code running in the stack).

This seems like something as simple as a type check on the returned stream would be sufficient:

if (!(is SslStream)) {
    /// negotiate TLS in the usual, existing way
}

I'm happy to propose a PR with a fix for this issue along with corresponding tests.

[ghost]

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

[ghost]

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

[brporter]

I've proposed a PR with a fix, along with a test. (#42473)

[geoffkizer]

ConnectCallback isn't intended to support this. It just lets you control how the connection is established. SocketsHttpHandler still owns the TLS negotiation.

I do think it's potentially interesting to allow you to control TLS negotiation as well, but I think we'd expose this as a separate hook.

Note that you can return an SslStream from ConnectCallback; you just can't replace the TLS negotiation logic. So you could specify an HTTP url and then actually establish a TLS connection under the covers if you want to.

[scalablecory]

Initial prototyping for System.Net.Connections exposed a HTTPS middleware concept that allowed exactly this, but we weren't able to find a good real-world need for it so we dropped it.

[mjsabby]

@geoffkizer this is our use-case as well, so I'm missing what is not possible.

[brporter]

@geoffkizer understanding that the intent of ConnectCallback was just for socket creation, the basic interface lends itself to all sorts of potential optimizations. Given that the existing implementation of ConnectCallback expects you to return a stream and not a socket, enabling a caller to perform their own TLS negotiation is natural. I think it's actually more surprising to users that I can't return any old stream, and that the stream I provide has to be in a certain state.

This fixes that by removing any preconditions on the returned stream (for TLS anyway, although I don't see others in the code).

[mjsabby]

I completely agree with @brporter, it is expecting a Stream. SslStream implements a Stream and so why wouldn't it work?

Is this something we can fix for 5.0?

[geoffkizer]

I think it's actually more surprising to users that I can't return any old stream, and that the stream I provide has to be in a certain state.

Not sure what you mean by this. You can return any old stream. It doesn't have to be in any particular state.

What you can't do with this callback is replace the TLS establishment logic that occurs for HTTPS urls.

[mjsabby]

@geoffkizer @brporter's PR addresses this. Is that an acceptable change for let's say 6.0? If it is for 6.0, then I'd like to argue for its inclusion in 5.0 as well.

[scalablecory]

Here are the distinct features I see being discussed:

• ConnectCallback that we have today, its job is purely to establishes the connection, not to replace any part of the protocol (e.g. TLS establishment for an HTTPS URL).
• A PlaintextFilterCallback that allows shimming pre-encrypted data.
• An EstablishTlsCallback whose job it is to wrap a Stream in an SslStream.
• A ConnectWithTlsCallback which can perform the jobs of ConnectCallback and EstablishTlsCallback in a single shot.

EstablishTlsCallback is something that I don't know if there is a huge use for -- you can already give us custom SslClientAuthenticationOptions and so it's not clear what you could do here that is different. I guess we would pass that options into the callback and the user could modify it to whatever they need.

One of the scenarios I've heard for ConnectWithTlsCallback is to allow some sort of TLS hole punching or resource allocation prior to connecting, or to allow pooling of pre-connected, pre-authenticated streams. I don't know enough about this scenario to get into the details of it, though. If we supported this, we would need to ensure the callback has all the info it needs to make a decision on if TLS is needed or not (e.g. the connect might be coming for an HTTP or HTTPS URI)

@brporter's PR essentially turns ConnectCallback into ConnectWithTlsCallback, by feature detecting the returned Stream -- if it's already an SslStream it would assume the user did their own TLS.

[brporter]

We can try and dream up all the different ways someone might want to interpose between the application and the transport layer, or you can support an interface that specifically enables that imposition - streams (and streams that wrap streams).

A design that feature-detects TLS negotiation is sensible in my view because you handle the basic use case on behalf of a user. A user that's consuming the connect callback has specific transport requirements, and you should defer to the implementer the elements of the transport that the provider of that callback is interested in. In my case, I have specific performance requirements around low-latency scenarios, and so I need to be handing SocketsHttpHandler a stream that has had TLS pre-negotiated in my own specific way because I need to avoid the round trip costs of the TLS handshake.

The current design is non-obvious because you ask for a stream, then you opaquely manipulate that stream to meet your own stream-state requirements. TLS is a state of the transport. It's sensible to do this when the returned stream is not a TLS stream and the resource being accessed is clearly meant to be accessed via TLS, but it is not sensible when I've provided that for you.

To be clear, the scenarios I'm using (or abusing) ConnectCallback are entirely centered around performance of small-payload requests. Specifically:

1. Avoiding TCP negotiation and TLS negotiation as injurious to request / response latency.
2. Performing sentinel transactions on a pool of pre-created TCP connections in order to widen congestion windows speculatively.

None of this can be done with the current design. I can't perform any meaningful operation on the socket I generate for you to prepare it for a low-latency request / response scenario because I need to first negotiate TLS on the newly generated socket. It would require some strange server implementation that is willing to listen on a socket and perform TLS... optionally on the same socket.

[stephentoub]

and telemetry (streams returned by ConnectCallback can't effectively have their contents intercepted by other code running in the stack)

How does the proposed solution enable this? e.g. if you returned new TelemetryStream(new SslStream(...)), the proposed type check will fail.

On top of that, the handler actually needs to be able to know about how TLS was established, e.g. the negotiated app protocol. If it creates the SslStream itself, it can query it for the relevant info. If it gets back an arbitrary Stream, from where does it get that info? In the TelemetryStream example above, from where does it get it? If someone uses a TLS stream from bouncy castle, how does it get that info? And in all these cases, how does it know to avoid layering on its own TLS? It can type check for SslStream, but that's one case of many.

There are solutions to this, but they're not as simple as just special-casing one specific Stream-derived type. For example:

• The original proposed solution here involved a second delegate, a PlaintextFilter that would take in the stream (maybe the one returned from the connect callback, maybe an SslStream wrapping that, maybe something else like a Buffered stream, it doesn't matter) and return a stream wrapping it. This gives the implementation the option to handle TLS in the middle if needed.
• Add more API (writable properties) to the SocketsHttpConnectionContext that lets this TLS data be passed back to the caller such that if you added your own TLS, you could store into that everything the handler needed to know. It wouldn't need to type check or inspect the returned stream, just the concrete properties on the context object. (In the Connections version of all of this, this is a set of properties in the property bag.)

Now, in addition to those, it's possible to still say we could choose to special-case SslStream, acknowledging all the deficiencies and incompleteness of that. One of those deficiencies is inherent to it being a special-case: saying that SslStream really is special and it being returned directly is to be taken to mean that the callback established TLS and the handler must not. Personally, I'm ok with that, as I suspect it doesn't cut off any truly meaningful scenarios (and if it did, it could be addressed in the future with an additional property on the context that said "yeah, I returned an SslStream, but don't read anything into that, don't treat it specially.") This doesn't replace the need for one of the above solutions I mentioned (or something like them), but it could be a stop-gap / one-off behavior; I don't know how many of the scenarios we've looked at it really addresses... probably some of Bryan's, but certainly not the plaintext monitoring cases. It's also a simple change we might be able to get into .NET 5; anything involving new API is much less likely at this point (but still not impossible).

[geoffkizer]

the handler actually needs to be able to know about how TLS was established

It's more than that -- the SocketsHttpHandler needs to control certain aspects of how TLS is established. Specifically, we need to control what is sent for supported app protocols depending on whether we are trying to make an HTTP11 or HTTP2 connection.

There's also some complexity with proxy scenarios.

[geoffkizer]

@scalablecory, I agree with your list of "distinct features" above. A few small thoughts:

An EstablishTlsCallback whose job it is to wrap a Stream in an SslStream.

The returned stream here doesn't need to be an SslStream, and we shouldn't require that. It could be "TLS stream from bouncy castle".

A ConnectWithTlsCallback which can perform the jobs of ConnectCallback and EstablishTlsCallback in a single shot.

I think this allows you to do the PlaintextFilterCallback as well. Effectively, it allows you to fully replace connection establishment (at the cost of more complexity) while the other three callbacks allow you to replace specific aspects of connection establishment.

As such, I don't really love the name "ConnectWithTlsCallback", but not sure what would be better.

Another thing to consider here is connection pooling, and if/how we would provide hooks that allow you greater control over this. The hooks above give you control over how a connection is established, but they don't tell you anything or let you control when it is reused, or discarded, or how many connections are idle at a given time. I don't have a specific proposal here, but I think we should work through related scenarios (like the one described by @brporter above) and try to understand these better.

[geoffkizer]

To be clear, the scenarios I'm using (or abusing) ConnectCallback are entirely centered around performance of small-payload requests. Specifically:

1. Avoiding TCP negotiation and TLS negotiation as injurious to request / response latency.
2. Performing sentinel transactions on a pool of pre-created TCP connections in order to widen congestion windows speculatively.

Thanks for this info @brporter. Understanding your scenario is super useful toward designing a workable solution.

[stephentoub]

It's more than that -- the SocketsHttpHandler needs to control certain aspects of how TLS is established. Specifically, we need to control what is sent for supported app protocols depending on whether we are trying to make an HTTP11 or HTTP2 connection. There's also some complexity with proxy scenarios.

Yes, that all falls under the deficiencies I was referring to. But the knobs exist to control the HTTP version and proxy to the point where you know what these values need to be. In such situations, you could create a valid SslStream. And for the future, additional data could be added to the context to eliminate the need to guess. My point being, with little downside (mainly that we have an extra condition to document and maintain) we could special-case SslStream, and if it works for someone, great. If not, oh well, the complete solution is required.

(I don't know whether @brporter and @mjsabby could actually be successful with that. If not, then it's a good sign that it's not worthwhile. If yes, I'm not seeing the huge downside, other than timeline and bar and whatnot, which could thwart it as well.)

[scalablecory]

It seems like it'd be fine to support a SslStream return. I would update the connect() context with SslClientAuthenticationOptions and/or whatever else is needed to support that properly.

[brporter]

@geoffkizer Thank you for the illustration of using a completely alternative TLS stream implementation. Special-casing SslStream as a solution is my personal framework-myopia in play.

If the consuming API has specific requirements for the transport, when a ConnectCallback is provided why not defer all those requirements to the provider? For example, require someone who has supplied a ConnectCallback with providing an SslStream (or equivalent implementation) if the destination resource is intended to be accessed via HTTPS, or HTTP/2, etc.

I suppose I'm not describing a ConnectCallback, but rather a provider pattern for byte transports.

@stephentoub of course you are correct regarding wrapping streams and the type check, but I was actually thinking about designs that return streams inherited from SslStream, despite what I may have written earlier. I was thinking about this from the perspective of the framework being the only supplier of TLS streams. Geoff's comment on BouncyCastle helped clarify my thinking there.

[stephentoub]

Geoff's comment on BouncyCastle

Pretty sure Geoff was quoting me ;-)

[wfurt]

There still may be time in 7.0 for some improvements mentioned in #63851. It would be great if interested folks can get daily build (or new preview) and provide feedback.