System.Net.Security.Tests assertion failure: !newKey.IsInvalid

[elinor-fung]

From log:

Process terminated. Assertion failed.
!newKey.IsInvalid
   at System.Security.Cryptography.RSAOpenSsl.SetKey(SafeEvpPKeyHandle newKey) in /_/src/libraries/Common/src/System/Security/Cryptography/RSAOpenSsl.cs:line 657
   at System.Security.Cryptography.RSAOpenSsl..ctor(SafeEvpPKeyHandle pkeyHandle) in /_/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/RSAOpenSsl.cs:line 83
   at System.Security.Cryptography.X509Certificates.OpenSslX509CertificateReader.GetRSAPrivateKey() in /_/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/OpenSslX509CertificateReader.cs:line 558
   at System.Security.Cryptography.X509Certificates.CertificateExtensionsCommon.GetPrivateKey[T](X509Certificate2 certificate, Predicate`1 matchesConstraints) in /_/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/CertificateExtensionsCommon.cs:line 61
   at System.Security.Cryptography.X509Certificates.RSACertificateExtensions.GetRSAPrivateKey(X509Certificate2 certificate) in /_/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/RSACertificateExtensions.cs:line 27
   at System.Security.Cryptography.X509Certificates.Tests.Common.CertificateAuthority.BuildOcspResponse(ReadOnlyMemory`1 certId, ReadOnlyMemory`1 nonceExtension) in /_/src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/CertificateAuthority.cs:line 639
   at System.Security.Cryptography.X509Certificates.Tests.Common.RevocationResponder.HandleRequest(HttpListenerContext context, Boolean& responded) in /_/src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/RevocationResponder.cs:line 256
   at System.Security.Cryptography.X509Certificates.Tests.Common.RevocationResponder.HandleRequest(HttpListenerContext context) in /_/src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/RevocationResponder.cs:line 138
   at System.Security.Cryptography.X509Certificates.Tests.Common.RevocationResponder.<HandleRequest>b__29_0(HttpListenerContext state) in /_/src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/RevocationResponder.cs:line 105
   at System.Threading.ThreadPoolWorkQueue.Dispatch() in /_/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPoolWorkQueue.cs:line 1096
   at System.Threading.PortableThreadPool.WorkerThread.WorkerThreadStart() in /_/src/libraries/System.Private.CoreLib/src/System/Threading/PortableThreadPool.WorkerThread.cs:line 128

Build Information

Build: https://dev.azure.com/dnceng-public/cbb18261-c48f-4abb-8651-8cdcb5474649/_build/results?buildId=759922
Build error leg or test failing: System.Net.Security.Tests.WorkItemExecution
Pull request: #105673

Error Message

Fill the error message using step by step known issues guidance.

{
  "ErrorMessage": ["System.Net.Security.Tests", "Assertion failed", "!newKey.IsInvalid"],
  "ErrorPattern": "",
  "BuildRetry": false,
  "ExcludeConsoleLog": false
}

Known issue validation

Build: 🔎 https://dev.azure.com/dnceng-public/public/_build/results?buildId=759922
Error message validated: [System.Net.Security.Tests Assertion failed !newKey.IsInvalid]
Result validation: ✅ Known issue matched with the provided build.
Validation performed at: 7/30/2024 5:34:02 PM UTC

Report

Summary

24-Hour Hit Count	7-Day Hit Count	1-Month Count
0	0	0

[dotnet-policy-service]

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

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

[vcsjones]

@bartonjs possible race with GetRSAPrivateKey and Dispose with the new cert loader?

[bartonjs]

A race in the loader would be exceedingly weird, since anything it disposes would be before returning, and things wouldn't finalize if they were returned out. But taking a quick gander.

[bartonjs]

The workhorse for this is undoubtedly BuildPrivatePki (as nothing else instantiates RevocationResponder). It doesn't use PFX loading, just X.509-DER loading (after CertificateRequest does X.509-DER building) and cert.CopyWithPrivateKey(). So, I don't think loader changes could really be relevant.

System.Net.Security tests do call new X509Certificate2(cert.Export(Pfx)), but only on the EE cert, which wouldn't be answering revocation prompts.

The places that I see all have all of the objects well-captured in a using... but, this is networking... and that means it's possible that a request got fired off and was being handled on the "server side" concurrent with the test giving up and disposing the certificate and responder it made up for the request.

So... we can remove the assert; but otherwise I think that this is just an inherent state with the nature of asynchronous operations.

[vcsjones]

We have removed asserts in the past (e.g. #86503) where we have checked if a Handle is valid or not. The validity of the handle may not be what we expected in a multi-threaded scenario.

As long as we interact with the handle safely (marshaller or addref + release) we can probably just get rid of the assert and let the handle do its thing by throwing an ObjectDisposedException.

[jeffhandley]

@bartonjs clarified for me that this is unchanged in .NET 9, so I'm moving it to Future.

[vcsjones]

Fixed in main by #107932.