include more details in exception if remote certificate validation fails #40110

wfurt · 2020-07-29T23:41:10Z

This includes enum value in Authentication exception if SslStream does not like remote certificate.
Relevant parts are in ForceAuthenticationAsync() when CompleteHandshake() fails.
I originally only used sslPolicyErrors but that is pretty crude enum with three values.
So I decided to also use X509ChainStatusFlags is available.

While working on this, I noticed that we always allocated verification delegate and copy certificate there.
With that, null checkin In SecureChannel really did not make sense.

The wrapper had extra string? parameter but it was never used.
So I did little bit shuffling to avoid unnecessary allocations and hopefully simplify the logic.

fixes #2359

ghost · 2020-07-29T23:41:12Z

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

src/libraries/System.Net.Security/src/System/Net/Security/SecureChannel.cs

bartonjs · 2020-07-30T02:52:10Z

Looks generally reasonable to me, but I didn't pay a lot of attention to the structural flow changes.

geoffkizer · 2020-08-04T05:18:04Z

src/libraries/System.Net.Security/src/Resources/Strings.resx

+    <value>The remote certificate is invalid according to the validation procedure: {0}</value>
+  </data>
+  <data name="net_ssl_io_cert_custom_validation" xml:space="preserve">
+    <value>The remote certificate was rejected by RemoteCertificateValidationCallback.</value>


Nit re wording: The RemoteCertificateValidationCallback is provided by the user, right? Can we change this to something like "The remote certificate was rejected by the provided RemoteCertificateValidationCallback" or similar, so it's clearer what's going on here?

yes, this is user provided. Updated as suggested.

geoffkizer · 2020-08-04T05:27:37Z

src/libraries/System.Net.Security/src/System/Net/Security/SecureChannel.cs

                        _sslAuthenticationOptions.CheckCertName,
                        _sslAuthenticationOptions.IsServer,
                        _sslAuthenticationOptions.TargetHost);
                }

                if (remoteCertValidationCallback != null)
                {
-                    success = remoteCertValidationCallback(_sslAuthenticationOptions.TargetHost, remoteCertificateEx, chain, sslPolicyErrors);
+                    object? sender = _ssl;


What's this check needed for?

Also, it seems like the only place you use _ssl is for this check. If you're just using it to determine whether the SecureChannel is disposed, could you just use bool _disposed instead?

I was worried about race conditions. Since there is no lock, grabbing global value and testing it seemed like safest.
If I use _disposed, it can be modified between check and the passing it to the callback, right?

Ah, I see, you're passing it to the validation callback below. Ok, makes sense.

Here's my suggestion: Don't store the SslStream in _ssl. Just pass it into VerifyRemoteCertificate -- that seems to be the only place it's used.

I was thinking about it but I was not sure if it is OK to pass null to that callback. I think we would not do that in the past.
It would certainly simplify this.

In the past SecureChannel did not have any real clue about SslStream - and that make tracing more difficult as we deal with objects caller has no visibility to. This wrapper business is yet another case. (I did not eliminated all the cases - just the one I cared about for the remote certificate as that is most common people hit)
I was thinking about merging them to one - but that is bigger change that we should do right now. (and it may have other issues)

If you pass SslStream to this routine, then why would it be null? I'm not sure exactly where this is called from, but I guess I assumed it was from SslStream?

I was thinking about merging them to one - but that is bigger change that we should do right now. (and it may have other issues)

Yep, merging seems good, but not right now.

it is called from within SslStream. But either with sync or async, somebody can get tired of waiting and call Dispose from other thread and move one. Maybe I'm just paranoid here and if somebody does, possible NRE inside of the callback would be acceptable. Our code really does not care AFAIK.
I was also thinking about just leaving _ssl on Dispose but I wanted to cut the circular reference.

If you pass the SslStream into this function, then they shouldn't get NRE, though they could get ODE. But I think that's fine, and unavoidable anyway since it's a race.

src/libraries/System.Net.Security/src/System/Net/Security/SecureChannel.cs

geoffkizer · 2020-08-04T14:50:34Z

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs

+                    else if (sslPolicyErrors == SslPolicyErrors.RemoteCertificateChainErrors && chainStatus != X509ChainStatusFlags.NoError)
+                    {
+                        // We failed only because of chain and we have some insight.
+                        SendAuthResetSignal(alertToken, ExceptionDispatchInfo.Capture(new AuthenticationException(SR.Format(SR.net_ssl_io_cert_validation, chainStatus), null)));


Should this have a different exception message than the case below? We go to the trouble of distinguishing them, so it seems like adding a bit more text to explain things is worthwhile.

We could but the enums do not have overlapping values so it would be always unique. If you think it is worth of changing, could you suggest come wording?

The existing message seems fine for the case below (sslPolicyErrors).

For the chain one, you could say something like "The remote certificate is invalid because of errors in the certificate chain: {0}" or something like that.

Up to you whether you think this is worth it.

geoffkizer

Generally looks good, a couple issues above.

wfurt · 2020-08-06T01:53:54Z

I resolved feedback as well as added little bit to fix #40402. Since I'm touching relevant area, doing separate PR would cause conflicts. The crux of the reported issue is that the recently added callback only updated values (like certificates and crypto) but ignored delegates. That is generally ok since there is no delegate to select certificate but in case of client cert authentication, one may still want to verify that vis custom code. (added two new tests for cases when verification callback is set via option and via SslStream constructor)

fixes #40402.

geoffkizer

LGTM

…ils (dotnet#40110) * include more details in exception if remote certificate validation fails * fix unit test linking * feedback from review * update exception message

include more details in exception if remote certificate validation fails

015510c

wfurt added the area-System.Net.Security label Jul 29, 2020

wfurt requested review from bartonjs and a team July 29, 2020 23:41

wfurt self-assigned this Jul 29, 2020

bartonjs reviewed Jul 30, 2020

View reviewed changes

src/libraries/System.Net.Security/src/System/Net/Security/SecureChannel.cs Outdated Show resolved Hide resolved

fix unit test linking

427f33e

geoffkizer reviewed Aug 4, 2020

View reviewed changes

src/libraries/System.Net.Security/src/System/Net/Security/SecureChannel.cs Show resolved Hide resolved

geoffkizer reviewed Aug 4, 2020

View reviewed changes

geoffkizer suggested changes Aug 4, 2020

View reviewed changes

feedback from review

936a4cb

geoffkizer approved these changes Aug 6, 2020

View reviewed changes

update exception message

f322b2f

jaredpar mentioned this pull request Aug 6, 2020

OSX machines are de-provisioned during CI / PR runs leading to failures #34472

Closed

wfurt merged commit 00d4741 into dotnet:master Aug 8, 2020

wfurt mentioned this pull request Aug 8, 2020

The RemoteCertificateValidationCallback is not respected when configured via a ServerOptionsSelectionCallback #40402

Closed

karelz added this to the 5.0.0 milestone Aug 18, 2020

ghost locked as resolved and limited conversation to collaborators Dec 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

include more details in exception if remote certificate validation fails #40110

include more details in exception if remote certificate validation fails #40110

wfurt commented Jul 29, 2020

ghost commented Jul 29, 2020

bartonjs commented Jul 30, 2020

geoffkizer Aug 4, 2020

wfurt Aug 6, 2020

geoffkizer Aug 4, 2020

wfurt Aug 4, 2020

geoffkizer Aug 4, 2020

geoffkizer Aug 4, 2020

wfurt Aug 4, 2020

geoffkizer Aug 4, 2020

wfurt Aug 4, 2020

geoffkizer Aug 4, 2020

geoffkizer Aug 4, 2020

wfurt Aug 6, 2020

geoffkizer Aug 6, 2020

geoffkizer left a comment

wfurt commented Aug 6, 2020

geoffkizer left a comment

include more details in exception if remote certificate validation fails #40110

include more details in exception if remote certificate validation fails #40110

Conversation

wfurt commented Jul 29, 2020

ghost commented Jul 29, 2020

bartonjs commented Jul 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoffkizer left a comment

Choose a reason for hiding this comment

wfurt commented Aug 6, 2020

geoffkizer left a comment

Choose a reason for hiding this comment