do not call RemoteCertificateValidationCallback on the same certificate again

[wfurt]

This is fix fix for #42823 to make the verification more consistent. On Windows, the callback can be triggered by sending data as the handshake is not quite finished when AuthenticateAs*() returns.

I'm not sure if the X509Certificate.Equals is sufficient but for now I assume so since the peer is already verified and trusted.

While testing this I bump to

Process terminated. Assertion failed.
     at System.Net.Http.ConnectHelper.<>c__DisplayClass3_0.<EstablishSslConnectionAsync>b__0(Object sender, X509Certificate certificate, X509Chain chain, SslPo
  licyErrors sslPolicyErrors) in C:\Users\toweinfu\github\wfurt-runtime\src\libraries\System.Net.Http\src\System\Net\Http\SocketsHttpHandler\ConnectHelper.cs:l
  ine 84
     at System.Net.Security.SecureChannel.VerifyRemoteCertificate(RemoteCertificateValidationCallback remoteCertValidationCallback, ProtocolToken& alertToken,
  SslPolicyErrors& sslPolicyErrors, X509ChainStatusFlags& chainStatus) in C:\Users\toweinfu\github\wfurt-runtime\src\libraries\System.Net.Security\src\System\N
  et\Security\SecureChannel.cs:line 1006
     at System.Net.Security.SslStream.CompleteHandshake(ProtocolToken& alertToken, SslPolicyErrors& sslPolicyErrors, X509ChainStatusFlags& chainStatus) in C:\U
  sers\toweinfu\github\wfurt-runtime\src\libraries\System.Net.Security\src\System\Net\Security\SslStream.Implementation.cs:line 612
     at System.Net.Security.SslStream.ForceAuthenticationAsync[TIOAdapter](TIOAdapter adapter, Boolean receiveFirst, Byte[] reAuthenticationData, Boolean isApm
  ) in C:\Users\toweinfu\github\wfurt-runtime\src\libraries\System.Net.Security\src\System\Net\Security\SslStream.Implementation.cs:line 411
     at System.Runtime.CompilerServices.AsyncMethodBuilderCore.Start[TStateMachine](TStateMachine& stateMachine) in C:\Users\toweinfu\github\wfurt-runtime\src\
  libraries\System.Private.CoreLib\src\System\Runtime\CompilerServices\AsyncMethodBuilderCore.cs:line 42
     at System.Net.Security.SslStream.ForceAuthenticationAsync[TIOAdapter](TIOAdapter adapter, Boolean receiveFirst, Byte[] reAuthenticationData, Boolean isApm
  )

It seems like the HTTP callback is not ready to called multiple times. In this case it is lame, but if renegotiation is enabled this may happen. Since there is no field issue, I left it as it was.

[ghost]

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

[stephentoub]

It seems like the HTTP callback is not ready to called multiple times.

This is specific to HttpClientHandler (you're not hitting this when using SocketsHttpHandler directly, right?). The callback signature from HttpClientHandler expects the HttpRequestMessage to be passed in, so we need to capture the instance into the closure used for the underlying RemoteCertificateValidationCallback so that it can be in turn passed to the handler's ServerCertificateCustomValidationCallback. But as a result of capturing it in that way, the HttpRequestMessage would end up being referenced for the lifetime of the callback, thus keeping it alive and from being garbage collected. The fix for this issue (#36979) was to null out the field after the callback was invoked (#37021), with the assumption that it wouldn't be invoked again. If it is invoked again, then it triggers the assert put in place to validate it wasn't being invoked again.

So, we have a choice to make, either:

1. Ensure the callback is never invoked multiple times. But it sounds like that's not actually viable, that it's expected to be invoked multiple times in the face of renegotation if the certificate changes.
2. Allow null to be passed instead of the original message on renegotation. This is in fact what's happening today, and the nullable annotation on the parameter is wrong. But if we want to accept that, then a) we need to remove the assert, and b) we need to change the first argument of the callback from HttpRequestMessage to HttpRequestMessage? to signify that it can actually be null, so that callbacks guard against it if they happen to be using the request message.
3. Hold on to the message for the lifetime of the connection in case it may be needed for a subsequent callback invocation, essentially reverting Avoid HttpClientHandler.ServerCertificateCustomValidationCallback keeping first request message alive #37021.
4. Expose something on HttpClientHandler to allow this choice to be made by the dev, e.g. exposing AllowRenegotation. But as it would default to true as is currently the case, this choice alone is likely insufficient.

None of these options seem great, but (2) is probably the best. @geoffkizer? (@jeffhandley, FYI on nullable annotations)

[wfurt]

yes, this is good description. I was thinking about it more as I was reading your writeup @stephentoub and all the cases when renegotiation happens in practice are when server asks for client certificate later in the session. Aside from the TLS13 I'm trying to fix, I'm not sure there is practical case when server's certificate would change. Option 2 seems best to me. Keeping Debug.Assert is perhaps good choice as it would not be in production code and we would effectively pass in NULL - something applications can deal with. Cutting memory use is more important IMHO.
Since there is no field report perhaps we can keep it as it is now. Explainung HttpRequestMessage? may be difficult IMHO.

[geoffkizer]

I was thinking about it more as I was reading your writeup @stephentoub and all the cases when renegotiation happens in practice are when server asks for client certificate later in the session.

Yeah, this is my understanding as well. Can the server actually send a new certificate during renegotiation? Even if it can, this doesn't seem like it's common at all in practice. In other words, I'm not sure there's a real issue here.

That said, if there are cases where this can happen, I agree that option 2 is best (i.e. pass null).

Aside from the TLS13 I'm trying to fix, I'm not sure there is practical case when server's certificate would change.

In the TLS13 case, the cert isn't actually changing though, right? It's just that it's not available until slightly later than usual. So it should still get the HttpRequestMessage, right?

[wfurt]

no, the certificate is not changing @geoffkizer . But the renegotiation path assumed it could and always called the validation callback again.

[geoffkizer]

What if both are null? Is this possible? Should we call the callback again in that case?

[wfurt]

In that case we will fall-through and hit the "normal" case. SslPolicyErrors.RemoteCertificateNotAvailable will be set.
If peer certificate was required (and that is true on client side) that would lead to negotiation failure unless that was override by the callback for some reason. To prevent this form happening twice, we would probably need to add another property to track if this was already called. I simply use presence of certificate to detect that now but it can still probably happen on server side. It feels like unlikely corner case and it should not impact the SocketHttpClient nor be TLS 13 specific. The callback is only one way for app to know "something" e.g. renegotiation happened. So I think it is ok to be called multiple times in general case.

The interesting scenario IMHO would be TLS resume where session can be established without full exchange. I don't know if Schannel remembers the certificates from the previous session and if that generally works same way with OpenSSL. (resume currently not working on Linux but I'm hoping to fix that)