Skip to content

[release/8.0] Dispose the certificate chain elements with the chain #62994

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 5, 2025

Conversation

MackinnonBuck
Copy link
Member

@MackinnonBuck MackinnonBuck commented Jul 29, 2025

Backport of #62531 to release/8.0

Dispose the certificate chain elements with the chain

Fixes an issue in certificate authentication where certificates within a certificate chain were not getting directly disposed.

Description

Failing to dispose each certificate within an X509 chain can create significant GC pressure for applications that frequently perform TLS handshakes. While the previous disposal logic only disposed the X509Chain itself, this PR updates the logic to first enumerate and dispose each certificate in the chain directly.

Customer Impact

The original contribution was from a customer who determined that this issue has a severe negative performance impact on their large scale web application. See #62531 (comment).

Regression?

  • Yes
  • No

Risk

  • High
  • Medium
  • Low

The change is straightforward and follows an established disposal pattern.

Verification

  • Manual (required)
  • Automated

Packaging changes reviewed?

  • Yes
  • No
  • N/A

* Dispose the certificate chain elements with the chain

* Fix the missing brace

* Remove snarky comment.

* Add another choice using based on review feedback

* Styling fixes

---------

Co-authored-by: Mackinnon Buck <[email protected]>
@Copilot Copilot AI review requested due to automatic review settings July 29, 2025 20:02
Copy link
Contributor

@Copilot Copilot AI left a comment

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 addresses a resource management issue by ensuring that X509Certificate2 objects in certificate chains are properly disposed. The fix prevents potential memory leaks by manually disposing all certificate chain elements before disposing the chain itself.

Key changes:

  • Replace using declarations with try-finally blocks for X509Chain disposal
  • Add manual disposal of certificate chain elements before disposing the chain
  • Fix a syntax error (semicolon to closing brace)

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/Shared/CertificateGeneration/UnixCertificateManager.cs Adds proper disposal pattern for X509Chain and its elements, fixes syntax error
src/Security/Authentication/Certificate/src/CertificateAuthenticationHandler.cs Implements the same disposal pattern for certificate validation
Comments suppressed due to low confidence (1)

src/Shared/CertificateGeneration/UnixCertificateManager.cs:178

  • This appears to be fixing a syntax error where a semicolon was incorrectly used instead of a closing brace.
        }

@dotnet-policy-service dotnet-policy-service bot added this to the 8.0.x milestone Jul 29, 2025
@MackinnonBuck MackinnonBuck added the Servicing-consider Shiproom approval is required for the issue label Jul 29, 2025
@MackinnonBuck MackinnonBuck added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Jul 30, 2025
@wtgodbe wtgodbe merged commit a6efb8b into release/8.0 Aug 5, 2025
23 of 25 checks passed
@wtgodbe wtgodbe deleted the mbuck/backport-cert-fix-to-8.0 branch August 5, 2025 01:05
@dotnet-policy-service dotnet-policy-service bot modified the milestones: 8.0.x, 8.0.20 Aug 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Servicing-approved Shiproom has approved the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants