Runtime: HTTP2: SocketsHttpHandler setting Ssl.AllowRenegotiation=false incorrectly

[davidsh]

Found during PR dotnet/corefx#35240.

SocketsHttpHandler (as part of always trying HTTP/2 first) is always setting SslStream.AllowRenegotiation to false when first attempting to connect via HTTP/2. This is fine
if the connection is truly HTTP/2 since HTTP/2 doesn't support TLS renegotiation. However, if the
remote endpoint doesn't support HTTP/2 and thus connects with HTTP/1.1, then the SslStream should
not have AllowRenegotiation set to false.

Client certificate scenarios frequently have the server ask for the client certificate later in the connection process. So, TLS renegotiation is used.

SocketsHttpHandler (or SslStream) should be fixed to reset the SslStream.AllowRenegotation property back to true (which is the default value) in cases where HTTP/1.1 is the resulting connection.

[davidsh]

@dotnet/ncl

[karelz]

Should be simple to do what @davidsh suggests above: SocketsHttpHandler (or SslStream) should be fixed to reset the SslStream.AllowRenegotation property back to true (which is the default value) in cases where HTTP/1.1 is the resulting connection.

Should be 2 line change + tests (which may be a bit tricky).

[davidkaya]

I don't know whether this is that easy (I may have overlooked something though). The check whether SslStream connected using HTTP/1.1 (even though wanted as HTTP/2) is already available at

https://github.com/dotnet/corefx/blob/d9cdbc3cca0f75e5d7692f9ae03978dd0ed9b2e9/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs#L434-L468

And there we have the SslStream available but SslStream itself does not allow to change AllowRenegotiation (there is no such API as SslStream.AllowRenegotiation) and also it does not provide access to underlying SSL options.

SSL options are initially passed to the SslStream through

https://github.com/dotnet/corefx/blob/d9cdbc3cca0f75e5d7692f9ae03978dd0ed9b2e9/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectHelper.cs#L157-L167

That would leave us to change the AllowRenegotiation from within the SslStream, is the SslStream the correct place to make such change based on the negotiated protocol?

[stephentoub]

Should be 2 line change + tests

I believe @davidkaya is correct. I don't see a good way to do this without either a) adding some API to help with it, b) changing the behavior of a previously shipped API (e.g. making SslStream re-query the passed in options), c) tearing down every downgraded connection and starting a new one (which would be bad), or d) using private reflection (ugh).

[davidsh]

I vote for b). The 'AllowRenegotiation' property is pretty new. I don't think having SslStream re-query it will break anyone.

[stephentoub]

I don't think having SslStream re-query it will break anyone.

It's more about the model. The idea was that SslStream would copy out of the options instance whatever it needed, so that the options instance could then be changed and used with other SslStreams. Whether or not anyone will do that, I don't know, but this breaks that model.

cc: @bartonjs, @krwq

[krwq]

Perhaps we should have a way of changing that in the ALPN callback or re-query after calling that callback (possibly new API).

On the other hand it slightly feels to me that since TLS is aware of ALPN it should know that HTTP2 does not allow re-negotiation in which case TLS would/should auto-restrict re-negotiation when it notices HTTP2 (in which case the HTTP client wouldn't do anything special to handle that).

[davidsh]

It's more about the model. The idea was that SslStream would copy out of the options instance whatever it needed, so that the options instance could then be changed and used with other SslStreams. Whether or not anyone will do that, I don't know, but this breaks that model.

I don't have a strong feeling about whether we should add a new API to fix this or adjust the previous behavior.

But I do have a strong feeling that we should fix this scenario.

[krwq]

IMO the simplest fix for this release (does not require new API) would be directly in SslStream and then if we ever find ourselves in a place where it would be useful to have more control over this behavior (I can't currently think of scenario other than HTTP2) then we could add a new API and possibly adjust HTTP client behavior.

Regardless I think testing this will be very tricky. Possibly @bartonjs has some ideas

[davidsh]

Regardless I think testing this will be very tricky. Possibly @bartonjs has some ideas

I wish we had APIs in SslStream (server-side) to cause renegotiation in the first place. We currently don't have that kind of control. So, we end up having to use servers that do it such as Windows IIS etc.

[stephentoub]

IMO the simplest fix for this release (does not require new API) would be directly in SslStream

You mean breaking layering and teaching SslStream about specifics of HTTP2?

[davidsh]

You mean breaking layering and teaching SslStream about specifics of HTTP2?

I agree that we should not break layering. But we need a way of SslStream to adjust the AllowRenegotiation or perhaps get rid of it entirely. What if SslStream client-side didn't care if server asked for renegotiation? We used to always allow it. What is the big deal if a non-comformant HTTP/2 server asked for it?

Perhaps this 'AllowRenegotiation' property is something we shouldn't have added in the first place.

[stephentoub]

What is the big deal if a non-comformant HTTP/2 asked for it?

That's interesting. In other words, fix this by simply removing the setting of AllowRenegotation = false in SocketsHttpHandler.

[stephentoub]

fix this by simply removing the setting of AllowRenegotation = false in SocketsHttpHandler

Essentially we'd be trading one bug for another. By setting AllowRenegotiation to false, we respect the HTTP/2 spec and treat renegotation requests as errors, but if we downgrade to HTTP/1.1 then valid renegotation requests will be denied. By not setting AllowRenegotation to false, we respect erroneous renegotation requests from a non-complaint server, and if we downgrade to HTTP/1.1 then valid renegotation requests will also be respected.

If we're going to have a bug, it would seem better for it to be more lenient in what we accept, i.e. all valid scenarios work and some things that we "should" reject we instead allow. My only question would be if there are security issues that arise from allowing this. @bartonjs? @GrabYourPitchforks? If there aren't any concerns there, I agree the right change for 3.0 is to simply remove the setting of AllowRenegotation = false.

[wfurt]

There is somewhat more than this. There are also requirements about ciphers and key sizes. https://http2.github.io/http2-spec/#rfc.section.9.2.2
That part is SHOULD NOT so we may deal with it later.

Perhaps ALPN based crypto profiles may not be bad option. That would allow proper HTTP2 settings.
As minimum we may have more work in future besides simply allowing TLS to negotiate.

[stephentoub]

There are also requirements about ciphers and key sizes.

That's a separate issue, though, right? If we don't have one open, can you open one?

[wfurt]

yes. The AllowRenegotiation has probably
biggest practical impact as it can break client side authentication. SocketsHttpHandler can take SslOptions. We may respect that if provided to give control over compat vs security tradeoffs. (and perhaps choose compat for HttpClient) similar to dotnet/corefx#36555.

[bartonjs]

I think I'm okay with being lax about the renegotation for now. In the distant future, when everyone's on TLS 1.3, it won't matter anyways.

[stephentoub]

Ok, thanks. Let's go ahead with it.

[geoffkizer]

For what it's worth, it's always possible for someone to pass their own SslOptions to disallow renegotiation if they really care about this. So if someone complains, we can just tell them to do that.

[stephentoub]

Good point.