Skip to content

Defer RemoteCertificate assignment after X509 Chain build#114781

Merged
rzikm merged 2 commits into
dotnet:mainfrom
rzikm:sslstream-defer-cert-storage
May 5, 2025
Merged

Defer RemoteCertificate assignment after X509 Chain build#114781
rzikm merged 2 commits into
dotnet:mainfrom
rzikm:sslstream-defer-cert-storage

Conversation

@rzikm

@rzikm rzikm commented Apr 17, 2025

Copy link
Copy Markdown
Member

Defer the assignment of the remote certificate until after the X509 chain validation process, this prevents weird exceptions being thrown if SslStream is disposed in parallel with chain building, as in:

    System.Net.Security.Tests.SslStreamDisposeTest.Dispose_ParallelWithHandshake_ThrowsODE [FAIL]
      System.ArgumentException : The chain context handle is invalid. (Parameter 'certificate')
      Stack Trace:
        /_/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/X509Chain.cs(99,0): at System.Security.Cryptography.X509Certificates.X509Chain.Build(X509Certificate2 certificate, Boolean throwOnException)
        /_/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/X509Chain.cs(91,0): at System.Security.Cryptography.X509Certificates.X509Chain.Build(X509Certificate2 certificate)
        /_/src/libraries/Common/src/System/Net/Security/CertificateValidation.Unix.cs(22,0): at System.Net.Security.CertificateValidation.BuildChainAndVerifyProperties(X509Chain chain, X509Certificate2 remoteCertificate, Boolean checkCertName, Boolean _, String hostName)
        /_/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Protocol.cs(1099,0): at System.Net.Security.SslStream.VerifyRemoteCertificate(RemoteCertificateValidationCallback remoteCertValidationCallback, SslCertificateTrust trust, ProtocolToken& alertToken, SslPolicyErrors& sslPolicyErrors, X509ChainStatusFlags& chainStatus)
        /_/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.IO.cs(579,0): at System.Net.Security.SslStream.CompleteHandshake(ProtocolToken& alertToken, SslPolicyErrors& sslPolicyErrors, X509ChainStatusFlags& chainStatus)
        /_/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.IO.cs(592,0): at System.Net.Security.SslStream.CompleteHandshake(SslAuthenticationOptions sslAuthenticationOptions)
        /_/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.IO.cs(379,0): at System.Net.Security.SslStream.ForceAuthenticationAsync[TIOAdapter](Boolean receiveFirst, Byte[] reAuthenticationData, CancellationToken cancellationToken)
        /_/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamDisposeTest.cs(150,0): at System.Net.Security.Tests.SslStreamDisposeTest.<Dispose_ParallelWithHandshake_ThrowsODE>g__ValidateExceptionAsync|3_1(Task task)
        /_/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamDisposeTest.cs(142,0): at System.Net.Security.Tests.SslStreamDisposeTest.<>c__DisplayClass3_0.<<Dispose_ParallelWithHandshake_ThrowsODE>b__0>d.MoveNext()
        --- End of stack trace from previous location ---
        /_/src/libraries/System.Threading.Tasks.Parallel/src/System/Threading/Tasks/Parallel.ForEachAsync.cs(301,0): at System.Threading.Tasks.Parallel.<>c__53`1.<<ForEachAsync>b__53_0>d.MoveNext()
        --- End of stack trace from previous location ---
        /_/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamDisposeTest.cs(115,0): at System.Net.Security.Tests.SslStreamDisposeTest.Dispose_ParallelWithHandshake_ThrowsODE()
        --- End of stack trace from previous location ---

Copilot AI review requested due to automatic review settings April 17, 2025 13:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR defers the assignment of the remote certificate until after building the X509 chain to prevent exceptions when SslStream is disposed in parallel with chain validation.

  • Update the null-check to directly evaluate the certificate parameter rather than the previously assigned _remoteCertificate.
  • Adjust the order of the _remoteCertificate assignment to occur after chain validation and update the remote callback usage accordingly.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

@rzikm

rzikm commented Apr 17, 2025

Copy link
Copy Markdown
Member Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@wfurt wfurt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rzikm rzikm merged commit 11afd86 into dotnet:main May 5, 2025
@github-actions github-actions Bot locked and limited conversation to collaborators Jun 4, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants