[release/10.0] Fix certificate disposal in RemoteCertificateValidationCallback causing NRE on reuse#124584
Open
github-actions[bot] wants to merge 5 commits intorelease/10.0from
Open
[release/10.0] Fix certificate disposal in RemoteCertificateValidationCallback causing NRE on reuse#124584github-actions[bot] wants to merge 5 commits intorelease/10.0from
github-actions[bot] wants to merge 5 commits intorelease/10.0from
Conversation
Only cleanup ExtraStore and ChainElements certificates when no RemoteCertificateValidationCallback is provided. When a callback is provided, users might add their own certificates to ExtraStore or keep references to certificates from ChainElements, and these should not be disposed by SslStream. Co-authored-by: rzikm <32671551+rzikm@users.noreply.github.com>
Changed shared certificate to use 'using' declaration to ensure proper cleanup even if test fails. Co-authored-by: rzikm <32671551+rzikm@users.noreply.github.com>
…nused variable - Refactored test to use a for loop instead of duplicating connection code - Fixed unused 'built' variable by discarding return value - Removed issue reference from test comment as requested Co-authored-by: rzikm <32671551+rzikm@users.noreply.github.com>
Added verification that certificates from chain.ChainElements are not disposed when RemoteCertificateValidationCallback is provided, as suggested by @bartonjs. The test now: - Saves certificates from ChainElements on first connection - Verifies they remain valid after connection closes - Verifies they remain valid after multiple connections Co-authored-by: rzikm <32671551+rzikm@users.noreply.github.com>
Contributor
|
Tagging subscribers to this area: @dotnet/ncl, @bartonjs, @vcsjones |
Member
|
cc @karelz |
ManickaP
approved these changes
Feb 19, 2026
|
Hi, is there any update on when this might be released? Thanks |
Member
The earliest this can be released is mid-April. The cut-off date for the next servicing release is next week. It is on my radar to get this merged in time for that. |
karelz
approved these changes
Mar 6, 2026
Member
|
Regression in 10, we should fix it in servicing given it impacts customer scenarios without a reasonable workaround. |
Member
|
Approved by Tactics (@jeffschwMSFT) via email on Fri 3/6. Adding Servicing-approved label accordingly. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backport of #123875 to release/10.0
/cc @rzikm @copilot
Fixes #123058
Customer Impact
Reported by customer in #123058
When customer provided custom RemoteCertificateValidationCallback during which they added extra certs to the X509Chain.ExtraStore, these certs would end up disposed by the SslStream, preventing their reuse across multiple connections.
Unless a workaround is implemented by the customer, the bug render's the application/server unable to establish new TLS connections.
Regression
Yes - introduced in PR #117667 in .NET 10.
The original change was an attempt to improve deterministic cleanup of resources. Adding more intermediate certs during cert validation callback wasn't considered common scenario and there was missing code coverage.
Testing
Targeted unit test added to ensure we returned to the previous behavior.
Risk
Low, the issue is well understood and the fix is simple.