SSLStream should support cancelling certificate chain building with cancellation token

[davidfowl]

Certificate chain building and verification should be cancellable AuthenticateAs* on OSX and Linux:

• runtime/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs

  Line 325 in b44492d

  X509Chain chain = TLSCertificateExtensions.BuildNewChain(

• runtime/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs

  Line 213 in 4bdf468

  chain = TLSCertificateExtensions.BuildNewChain(cert, includeClientApplicationPolicy: false);

[ghost]

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq
Notify danmosemsft if you want to be subscribed.

[bartonjs]

It's certainly not cancellable on macOS, since neither SecTrustEvaluate or SecTrustEvaluateWithError are capable of being cancelled.

[bartonjs]

Well, SslStream could build the chain via Task.Run, I suppose.

[ghost]

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

[stephentoub]

Well, SslStream could build the chain via Task.Run, I suppose.

That would effectively cancel the wait but not the actual operation.

When the caller gets back control after the resulting OperationCanceledException, what happens if they then dispose stuff that's being used? Does anything break, or is everything properly protected by SafeHandles?

Or what happens if they retry the operation while the previous operation is still in flight?

Also, how long do these operations typically take, and are they parallelizable? If you are considering using async-over-sync, I'm wondering if it makes sense to use one or more dedicated threads rather than using the thread pool.

[bartonjs]

When the caller gets back control after the resulting OperationCanceledException, what happens if they then dispose stuff that's being used?

Any certificates used as input should be being used as SafeHandles inside the execution already to prevent system corruption from a parallel/external dispose. Disposing the chain object will do nothing if it hasn't yet completed.

Or what happens if they retry the operation while the previous operation is still in flight?

If it's on the same X509Chain instance, both operations will then assign the ChainElements, ChainStatus, and SafeHandle properties. So the same object should not be reused (aka it's definitely not thread safe).

how long do these operations typically take

Sub-hundred milliseconds. Except when they need the network. Then sub-hundred-milliseconds + network time (where E(x) is low, but Theta(x) is chainPolicy.UrlRetrievalTimeout (default: effectively infinite) ... except UrlRetrievalTimeout isn't respected on macOS).

are they parallelizable?

Yep

[davidfowl]

The manual fetches (on OSX and Linux) could also be fully asynchronous in the SSL Stream case but I didn't want to pollute this issue with that 😄

[bartonjs]

There aren't manual fetches on macOS. Only Linux.

[wfurt]

should we close this in favor of #35844? If we have options to pass complete chain to SslStream this should not be big issue, right?

[davidfowl]

We can avoid making the call completely right? We can also set a timeout if we choose to resolve ourselves yes?

[wfurt]

yes, I think so, at least for cases in our control. X509ChainPolicy.UrlRetrievalTimeout is what you can use to control timeout if you choose X509Chain to build/fetch it for you. I did quick search through runtime repo and I did not see test verifying the expiration.
I think we can fix that before closing this issue. But there would be no need for new API so I would do it as time permits.

[vcsjones]

I don't know if this is still correct but I was under the impression that UrlRetrievalTimeout is ignored on MacOS.

https://github.com/dotnet/runtime/blob/4f9ae42d861fcb4be2fcd5d3d55d5f227d30e723/docs/design/features/cross-platform-cryptography.md#x509chain

macOS does not support a user-initiated timeout on CRL/OCSP/AIA downloading, so X509ChainPolicy.UrlRetrievalTimeout is ignored.

[wfurt]

may not be "in our control". That still may not matter if full chain is provided.

[karelz]

Triage: Verify it works correctly, then close.

[wfurt]

I was planning to close this with #35844 "pre-validated immutable full certificate chain" and add test to verify that once certificate context was built. (Both calls to BuildNewChain @davidfowl mentioned are gone)

That proved to be challenging as even if the server does not do I/O, client will and there is no good way how to separate client and server with current test infrastructure.
Also #35844 is focused on server part e.g. loading chain from a file or pushing responsibility to caller.
However recent incident brought up case with client certificates. Since that is out of server's domain, it can be anything, possibly forcing server to contact malicious server.

I think this should be revisited beyond just the cancelation. I think we should find a better way how to expose verification options. Perhaps adding option to just give certificate to callback or exposing Timeout (and making that as xplat as we can)
Since the main use case was address with #35844 and there is currently no good technical way how to pass cancellation around and make that reasonably platform agnostic, I'm going to push it out of 5.0.

[wfurt]

Client and server now have option to pass in X509ChainPolicy via CertificateChainPolicy in ssl options. If present, chain constructed inside SslStream will respect the setting including DisableCertificateDownloads and UrlRetrievalTimeout .