Fix SslAuthenticationOptions incorrectly disposing unowned IntermediateCertificates

[Copilot]

SslAuthenticationOptions.UpdateOptions failed to reset OwnsCertificateContext = false when a caller-provided ServerCertificateContext replaced a previously self-owned context. If UpdateOptions was called twice on the same instance — first with only a ServerCertificate (setting OwnsCertificateContext = true), then with a caller-owned ServerCertificateContext — subsequent Dispose() would call ReleaseResources() on the caller's context, disposing its intermediate certificates and breaking cached contexts for future requests.

Changes

• SslAuthenticationOptions.cs — In the ServerCertificateContext != null branch of UpdateOptions:

  • Release the previously-owned context (if any) to avoid resource leak
  • Set OwnsCertificateContext = false so Dispose() won't touch the caller's context

• SslAuthenticationOptionsTests.cs — Regression test using ECDsa P-256 certs that:

  • Simulates two UpdateOptions calls (bare cert → caller context)
  • Asserts OwnsCertificateContext == false and the correct context is set after the second call
  • Verifies the previously-owned context's intermediate is disposed (Handle == IntPtr.Zero)
  • Verifies the caller's intermediates survive Dispose() via Export()

• FixesSslAuthenticationOptions Incorrectly Disposes Unowned IntermediateCertificates #131748

[Copilot]

Copilot was unable to review this pull request because there is no eligible user to bill. To allow Copilot reviews on bot-authored pull requests, enable direct organization billing in your organization's Copilot settings.

[azure-pipelines]

Azure Pipelines:
16 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

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

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/libraries/System.Net.Security/tests/UnitTests/SslAuthenticationOptionsTests.cs:222

• The post-Dispose validation is weak (Subject may still succeed even if the cert handle was freed). Consider asserting via an operation that requires the native handle (e.g., Export) and ensure the test releases the callerContext intermediates to avoid leaking handles across the test run (use ReleaseResources in a finally).

// Verify that the caller's intermediate certificates were not disposed
Assert.True(callerContext.IntermediateCertificates.Count > 0, "Expected at least one intermediate in the caller's context.");

[Copilot]

Pull request overview

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

Suppressed comments (2)

src/libraries/System.Net.Security/src/System/Net/Security/SslAuthenticationOptions.cs:158

• When switching to a caller-provided ServerCertificateContext, UpdateOptions unconditionally releases the previously owned CertificateContext. If the caller passes the same context instance that is currently in CertificateContext (possible in internal flows), this will dispose the intermediates of the context you’re about to keep, potentially breaking the handshake. TlsSession.SetSessionCertificateContext guards against this with a ReferenceEquals check; UpdateOptions should do the same.

 // Release any previously owned context before replacing it with the caller's context.
if (OwnsCertificateContext && CertificateContext != null)
{
CertificateContext.ReleaseResources();

src/libraries/System.Net.Security/tests/UnitTests/SslAuthenticationOptionsTests.cs:202

• This test’s caller-owned context is built with only the intermediate in additionalCertificates. On platforms where chain building requires a trusted root (see SslStreamCertificateContext.Create’s ChainBuildNeedsTrustedRoot retry), chain.Build can yield zero elements on failure and IntermediateCertificates can end up empty, making the test platform-dependent. Including the root certificate in additionalCertificates makes the chain construction deterministic across platforms.

 // Create a caller-owned context with an intermediate certificate
SslStreamCertificateContext callerContext = SslStreamCertificateContext.Create(
leafWithKey,
new X509Certificate2Collection { intermediateWithKey },
offline: true);

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/libraries/System.Net.Security/tests/UnitTests/SslAuthenticationOptionsTests.cs:235

• callerContext holds native crypto handles and cloned certificates (e.g., on Linux SafeX509Handle/SafeEvpPKeyHandle). The test verifies it is not disposed, but then leaves it alive until finalization, which can create unnecessary resource pressure across the test run. After the assertions, explicitly release the context resources.

 foreach (X509Certificate2 cert in callerContext.IntermediateCertificates)
{
Assert.Null(Record.Exception(() => cert.Export(X509ContentType.Cert)));
}

[Copilot]

Pull request overview

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

Suppressed comments (2)

src/libraries/System.Net.Security/src/System/Net/Security/SslAuthenticationOptions.cs:161

• When switching to a provided ServerCertificateContext, the code releases resources based solely on the current ownership bit. If the new context instance happens to be the same object as the current CertificateContext, this will dispose intermediates on the context that will continue to be used, leaving CertificateContext pointing at a released object. Guard the release with a ReferenceEquals check (similar to TlsSession.SetSessionCertificateContext).

 // Release any previously owned context before replacing it with the caller's context.
if (OwnsCertificateContext && CertificateContext is not null &&
!ReferenceEquals(CertificateContext, sslServerAuthenticationOptions.ServerCertificateContext))
{
CertificateContext.ReleaseResources();
}
CertificateContext = sslServerAuthenticationOptions.ServerCertificateContext;

src/libraries/System.Net.Security/tests/UnitTests/SslAuthenticationOptionsTests.cs:206

• ownedIntermediate is declared as non-nullable X509Certificate2 but is assigned null when the intermediate list is empty; with Nullable enabled in this test project, this will produce warnings (and may fail the build if treated as errors). Make the variable nullable.

 Assert.NotEmpty(ownedContext.IntermediateCertificates);
X509Certificate2 ownedIntermediate = ownedContext.IntermediateCertificates[0];
options.CertificateContext = ownedContext;

[Copilot]

Pull request overview

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

Suppressed comments (1)

src/libraries/System.Net.Security/src/System/Net/Security/SslAuthenticationOptions.cs:166

• When UpdateOptions is called repeatedly (e.g., via ServerOptionDelegate for multiple ClientHello frames), the ServerCertificate path can replace an existing owned CertificateContext without releasing the old one. This leaks intermediate cert handles/resources for each update where OwnsCertificateContext was true.

Consider capturing the previous owned context and calling ReleaseResources() after successfully creating/assigning the new context in the ServerCertificate branch, similar to what was added for the ServerCertificateContext branch.

 OwnsCertificateContext = false;
}
else if (sslServerAuthenticationOptions.ServerCertificate != null)
{
X509Certificate2? certificateWithKey = sslServerAuthenticationOptions.ServerCertificate as X509Certificate2;

[azure-pipelines]

Azure Pipelines:
Successfully started running 4 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[wfurt]

LGTM

[Copilot]

SslAuthenticationOptions.UpdateOptions failed to reset OwnsCertificateContext = false when a caller-provided ServerCertificateContext replaced a previously self-owned context. If UpdateOptions was called twice on the same instance — first with only a ServerCertificate (setting OwnsCertificateContext = true), then with a caller-owned ServerCertificateContext — subsequent Dispose() would call ReleaseResources() on the caller's context, disposing its intermediate certificates and breaking cached contexts for future requests.

Changes

• SslAuthenticationOptions.cs — In the ServerCertificateContext != null branch of UpdateOptions:

  • Release the previously-owned context (if any) to avoid resource leak
  • Set OwnsCertificateContext = false so Dispose() won't touch the caller's context

• SslAuthenticationOptionsTests.cs — Regression test using ECDsa P-256 certs that:

  • Simulates two UpdateOptions calls (bare cert → caller context)
  • Asserts OwnsCertificateContext == false and the correct context is set after the second call
  • Verifies the previously-owned context's intermediate is disposed (Handle == IntPtr.Zero)
  • Verifies the caller's intermediates survive Dispose() via Export()

• FixesSslAuthenticationOptions Incorrectly Disposes Unowned IntermediateCertificates #131748

[Copilot]

Copilot was unable to review this pull request because there is no eligible user to bill. To allow Copilot reviews on bot-authored pull requests, enable direct organization billing in your organization's Copilot settings.

[azure-pipelines]

Azure Pipelines:
16 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

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

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/libraries/System.Net.Security/tests/UnitTests/SslAuthenticationOptionsTests.cs:222

• The post-Dispose validation is weak (Subject may still succeed even if the cert handle was freed). Consider asserting via an operation that requires the native handle (e.g., Export) and ensure the test releases the callerContext intermediates to avoid leaking handles across the test run (use ReleaseResources in a finally).

// Verify that the caller's intermediate certificates were not disposed
Assert.True(callerContext.IntermediateCertificates.Count > 0, "Expected at least one intermediate in the caller's context.");

[Copilot]

Pull request overview

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

Suppressed comments (2)

src/libraries/System.Net.Security/src/System/Net/Security/SslAuthenticationOptions.cs:158

• When switching to a caller-provided ServerCertificateContext, UpdateOptions unconditionally releases the previously owned CertificateContext. If the caller passes the same context instance that is currently in CertificateContext (possible in internal flows), this will dispose the intermediates of the context you’re about to keep, potentially breaking the handshake. TlsSession.SetSessionCertificateContext guards against this with a ReferenceEquals check; UpdateOptions should do the same.

 // Release any previously owned context before replacing it with the caller's context.
if (OwnsCertificateContext && CertificateContext != null)
{
CertificateContext.ReleaseResources();

src/libraries/System.Net.Security/tests/UnitTests/SslAuthenticationOptionsTests.cs:202

• This test’s caller-owned context is built with only the intermediate in additionalCertificates. On platforms where chain building requires a trusted root (see SslStreamCertificateContext.Create’s ChainBuildNeedsTrustedRoot retry), chain.Build can yield zero elements on failure and IntermediateCertificates can end up empty, making the test platform-dependent. Including the root certificate in additionalCertificates makes the chain construction deterministic across platforms.

 // Create a caller-owned context with an intermediate certificate
SslStreamCertificateContext callerContext = SslStreamCertificateContext.Create(
leafWithKey,
new X509Certificate2Collection { intermediateWithKey },
offline: true);

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/libraries/System.Net.Security/tests/UnitTests/SslAuthenticationOptionsTests.cs:235

• callerContext holds native crypto handles and cloned certificates (e.g., on Linux SafeX509Handle/SafeEvpPKeyHandle). The test verifies it is not disposed, but then leaves it alive until finalization, which can create unnecessary resource pressure across the test run. After the assertions, explicitly release the context resources.

 foreach (X509Certificate2 cert in callerContext.IntermediateCertificates)
{
Assert.Null(Record.Exception(() => cert.Export(X509ContentType.Cert)));
}

[Copilot]

Pull request overview

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

Suppressed comments (2)

src/libraries/System.Net.Security/src/System/Net/Security/SslAuthenticationOptions.cs:161

• When switching to a provided ServerCertificateContext, the code releases resources based solely on the current ownership bit. If the new context instance happens to be the same object as the current CertificateContext, this will dispose intermediates on the context that will continue to be used, leaving CertificateContext pointing at a released object. Guard the release with a ReferenceEquals check (similar to TlsSession.SetSessionCertificateContext).

 // Release any previously owned context before replacing it with the caller's context.
if (OwnsCertificateContext && CertificateContext is not null &&
!ReferenceEquals(CertificateContext, sslServerAuthenticationOptions.ServerCertificateContext))
{
CertificateContext.ReleaseResources();
}
CertificateContext = sslServerAuthenticationOptions.ServerCertificateContext;

src/libraries/System.Net.Security/tests/UnitTests/SslAuthenticationOptionsTests.cs:206

• ownedIntermediate is declared as non-nullable X509Certificate2 but is assigned null when the intermediate list is empty; with Nullable enabled in this test project, this will produce warnings (and may fail the build if treated as errors). Make the variable nullable.

 Assert.NotEmpty(ownedContext.IntermediateCertificates);
X509Certificate2 ownedIntermediate = ownedContext.IntermediateCertificates[0];
options.CertificateContext = ownedContext;

[Copilot]

Pull request overview

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

Suppressed comments (1)

src/libraries/System.Net.Security/src/System/Net/Security/SslAuthenticationOptions.cs:166

• When UpdateOptions is called repeatedly (e.g., via ServerOptionDelegate for multiple ClientHello frames), the ServerCertificate path can replace an existing owned CertificateContext without releasing the old one. This leaks intermediate cert handles/resources for each update where OwnsCertificateContext was true.

Consider capturing the previous owned context and calling ReleaseResources() after successfully creating/assigning the new context in the ServerCertificate branch, similar to what was added for the ServerCertificateContext branch.

 OwnsCertificateContext = false;
}
else if (sslServerAuthenticationOptions.ServerCertificate != null)
{
X509Certificate2? certificateWithKey = sslServerAuthenticationOptions.ServerCertificate as X509Certificate2;

[azure-pipelines]

Azure Pipelines:
Successfully started running 4 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[wfurt]

LGTM

[Copilot]

SslAuthenticationOptions.UpdateOptions failed to reset OwnsCertificateContext = false when a caller-provided ServerCertificateContext replaced a previously self-owned context. If UpdateOptions was called twice on the same instance — first with only a ServerCertificate (setting OwnsCertificateContext = true), then with a caller-owned ServerCertificateContext — subsequent Dispose() would call ReleaseResources() on the caller's context, disposing its intermediate certificates and breaking cached contexts for future requests.

Changes

• SslAuthenticationOptions.cs — In the ServerCertificateContext != null branch of UpdateOptions:

  • Release the previously-owned context (if any) to avoid resource leak
  • Set OwnsCertificateContext = false so Dispose() won't touch the caller's context

• SslAuthenticationOptionsTests.cs — Regression test using ECDsa P-256 certs that:

  • Simulates two UpdateOptions calls (bare cert → caller context)
  • Asserts OwnsCertificateContext == false and the correct context is set after the second call
  • Verifies the previously-owned context's intermediate is disposed (Handle == IntPtr.Zero)
  • Verifies the caller's intermediates survive Dispose() via Export()

• FixesSslAuthenticationOptions Incorrectly Disposes Unowned IntermediateCertificates #131748

[Copilot]

Copilot was unable to review this pull request because there is no eligible user to bill. To allow Copilot reviews on bot-authored pull requests, enable direct organization billing in your organization's Copilot settings.

[azure-pipelines]

Azure Pipelines:
16 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

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

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/libraries/System.Net.Security/tests/UnitTests/SslAuthenticationOptionsTests.cs:222

• The post-Dispose validation is weak (Subject may still succeed even if the cert handle was freed). Consider asserting via an operation that requires the native handle (e.g., Export) and ensure the test releases the callerContext intermediates to avoid leaking handles across the test run (use ReleaseResources in a finally).

// Verify that the caller's intermediate certificates were not disposed
Assert.True(callerContext.IntermediateCertificates.Count > 0, "Expected at least one intermediate in the caller's context.");

[Copilot]

Pull request overview

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

Suppressed comments (2)

src/libraries/System.Net.Security/src/System/Net/Security/SslAuthenticationOptions.cs:158

• When switching to a caller-provided ServerCertificateContext, UpdateOptions unconditionally releases the previously owned CertificateContext. If the caller passes the same context instance that is currently in CertificateContext (possible in internal flows), this will dispose the intermediates of the context you’re about to keep, potentially breaking the handshake. TlsSession.SetSessionCertificateContext guards against this with a ReferenceEquals check; UpdateOptions should do the same.

 // Release any previously owned context before replacing it with the caller's context.
if (OwnsCertificateContext && CertificateContext != null)
{
CertificateContext.ReleaseResources();

src/libraries/System.Net.Security/tests/UnitTests/SslAuthenticationOptionsTests.cs:202

• This test’s caller-owned context is built with only the intermediate in additionalCertificates. On platforms where chain building requires a trusted root (see SslStreamCertificateContext.Create’s ChainBuildNeedsTrustedRoot retry), chain.Build can yield zero elements on failure and IntermediateCertificates can end up empty, making the test platform-dependent. Including the root certificate in additionalCertificates makes the chain construction deterministic across platforms.

 // Create a caller-owned context with an intermediate certificate
SslStreamCertificateContext callerContext = SslStreamCertificateContext.Create(
leafWithKey,
new X509Certificate2Collection { intermediateWithKey },
offline: true);

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/libraries/System.Net.Security/tests/UnitTests/SslAuthenticationOptionsTests.cs:235

• callerContext holds native crypto handles and cloned certificates (e.g., on Linux SafeX509Handle/SafeEvpPKeyHandle). The test verifies it is not disposed, but then leaves it alive until finalization, which can create unnecessary resource pressure across the test run. After the assertions, explicitly release the context resources.

 foreach (X509Certificate2 cert in callerContext.IntermediateCertificates)
{
Assert.Null(Record.Exception(() => cert.Export(X509ContentType.Cert)));
}

[Copilot]

Pull request overview

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

Suppressed comments (2)

src/libraries/System.Net.Security/src/System/Net/Security/SslAuthenticationOptions.cs:161

• When switching to a provided ServerCertificateContext, the code releases resources based solely on the current ownership bit. If the new context instance happens to be the same object as the current CertificateContext, this will dispose intermediates on the context that will continue to be used, leaving CertificateContext pointing at a released object. Guard the release with a ReferenceEquals check (similar to TlsSession.SetSessionCertificateContext).

 // Release any previously owned context before replacing it with the caller's context.
if (OwnsCertificateContext && CertificateContext is not null &&
!ReferenceEquals(CertificateContext, sslServerAuthenticationOptions.ServerCertificateContext))
{
CertificateContext.ReleaseResources();
}
CertificateContext = sslServerAuthenticationOptions.ServerCertificateContext;

src/libraries/System.Net.Security/tests/UnitTests/SslAuthenticationOptionsTests.cs:206

• ownedIntermediate is declared as non-nullable X509Certificate2 but is assigned null when the intermediate list is empty; with Nullable enabled in this test project, this will produce warnings (and may fail the build if treated as errors). Make the variable nullable.

 Assert.NotEmpty(ownedContext.IntermediateCertificates);
X509Certificate2 ownedIntermediate = ownedContext.IntermediateCertificates[0];
options.CertificateContext = ownedContext;

[Copilot]

Pull request overview

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

Suppressed comments (1)

src/libraries/System.Net.Security/src/System/Net/Security/SslAuthenticationOptions.cs:166

• When UpdateOptions is called repeatedly (e.g., via ServerOptionDelegate for multiple ClientHello frames), the ServerCertificate path can replace an existing owned CertificateContext without releasing the old one. This leaks intermediate cert handles/resources for each update where OwnsCertificateContext was true.

Consider capturing the previous owned context and calling ReleaseResources() after successfully creating/assigning the new context in the ServerCertificate branch, similar to what was added for the ServerCertificateContext branch.

 OwnsCertificateContext = false;
}
else if (sslServerAuthenticationOptions.ServerCertificate != null)
{
X509Certificate2? certificateWithKey = sslServerAuthenticationOptions.ServerCertificate as X509Certificate2;

[azure-pipelines]

Azure Pipelines:
Successfully started running 4 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[wfurt]

LGTM

[Copilot]

SslAuthenticationOptions.UpdateOptions failed to reset OwnsCertificateContext = false when a caller-provided ServerCertificateContext replaced a previously self-owned context. If UpdateOptions was called twice on the same instance — first with only a ServerCertificate (setting OwnsCertificateContext = true), then with a caller-owned ServerCertificateContext — subsequent Dispose() would call ReleaseResources() on the caller's context, disposing its intermediate certificates and breaking cached contexts for future requests.

Changes

• SslAuthenticationOptions.cs — In the ServerCertificateContext != null branch of UpdateOptions:

  • Release the previously-owned context (if any) to avoid resource leak
  • Set OwnsCertificateContext = false so Dispose() won't touch the caller's context

• SslAuthenticationOptionsTests.cs — Regression test using ECDsa P-256 certs that:

  • Simulates two UpdateOptions calls (bare cert → caller context)
  • Asserts OwnsCertificateContext == false and the correct context is set after the second call
  • Verifies the previously-owned context's intermediate is disposed (Handle == IntPtr.Zero)
  • Verifies the caller's intermediates survive Dispose() via Export()

• FixesSslAuthenticationOptions Incorrectly Disposes Unowned IntermediateCertificates #131748

[Copilot]

Copilot was unable to review this pull request because there is no eligible user to bill. To allow Copilot reviews on bot-authored pull requests, enable direct organization billing in your organization's Copilot settings.

[azure-pipelines]

Azure Pipelines:
16 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

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

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/libraries/System.Net.Security/tests/UnitTests/SslAuthenticationOptionsTests.cs:222

• The post-Dispose validation is weak (Subject may still succeed even if the cert handle was freed). Consider asserting via an operation that requires the native handle (e.g., Export) and ensure the test releases the callerContext intermediates to avoid leaking handles across the test run (use ReleaseResources in a finally).

// Verify that the caller's intermediate certificates were not disposed
Assert.True(callerContext.IntermediateCertificates.Count > 0, "Expected at least one intermediate in the caller's context.");

[Copilot]

Pull request overview

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

Suppressed comments (2)

src/libraries/System.Net.Security/src/System/Net/Security/SslAuthenticationOptions.cs:158

• When switching to a caller-provided ServerCertificateContext, UpdateOptions unconditionally releases the previously owned CertificateContext. If the caller passes the same context instance that is currently in CertificateContext (possible in internal flows), this will dispose the intermediates of the context you’re about to keep, potentially breaking the handshake. TlsSession.SetSessionCertificateContext guards against this with a ReferenceEquals check; UpdateOptions should do the same.

 // Release any previously owned context before replacing it with the caller's context.
if (OwnsCertificateContext && CertificateContext != null)
{
CertificateContext.ReleaseResources();

src/libraries/System.Net.Security/tests/UnitTests/SslAuthenticationOptionsTests.cs:202

• This test’s caller-owned context is built with only the intermediate in additionalCertificates. On platforms where chain building requires a trusted root (see SslStreamCertificateContext.Create’s ChainBuildNeedsTrustedRoot retry), chain.Build can yield zero elements on failure and IntermediateCertificates can end up empty, making the test platform-dependent. Including the root certificate in additionalCertificates makes the chain construction deterministic across platforms.

 // Create a caller-owned context with an intermediate certificate
SslStreamCertificateContext callerContext = SslStreamCertificateContext.Create(
leafWithKey,
new X509Certificate2Collection { intermediateWithKey },
offline: true);

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/libraries/System.Net.Security/tests/UnitTests/SslAuthenticationOptionsTests.cs:235

• callerContext holds native crypto handles and cloned certificates (e.g., on Linux SafeX509Handle/SafeEvpPKeyHandle). The test verifies it is not disposed, but then leaves it alive until finalization, which can create unnecessary resource pressure across the test run. After the assertions, explicitly release the context resources.

 foreach (X509Certificate2 cert in callerContext.IntermediateCertificates)
{
Assert.Null(Record.Exception(() => cert.Export(X509ContentType.Cert)));
}

[Copilot]

Pull request overview

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

Suppressed comments (2)

src/libraries/System.Net.Security/src/System/Net/Security/SslAuthenticationOptions.cs:161

• When switching to a provided ServerCertificateContext, the code releases resources based solely on the current ownership bit. If the new context instance happens to be the same object as the current CertificateContext, this will dispose intermediates on the context that will continue to be used, leaving CertificateContext pointing at a released object. Guard the release with a ReferenceEquals check (similar to TlsSession.SetSessionCertificateContext).

 // Release any previously owned context before replacing it with the caller's context.
if (OwnsCertificateContext && CertificateContext is not null &&
!ReferenceEquals(CertificateContext, sslServerAuthenticationOptions.ServerCertificateContext))
{
CertificateContext.ReleaseResources();
}
CertificateContext = sslServerAuthenticationOptions.ServerCertificateContext;

src/libraries/System.Net.Security/tests/UnitTests/SslAuthenticationOptionsTests.cs:206

• ownedIntermediate is declared as non-nullable X509Certificate2 but is assigned null when the intermediate list is empty; with Nullable enabled in this test project, this will produce warnings (and may fail the build if treated as errors). Make the variable nullable.

 Assert.NotEmpty(ownedContext.IntermediateCertificates);
X509Certificate2 ownedIntermediate = ownedContext.IntermediateCertificates[0];
options.CertificateContext = ownedContext;

[Copilot]

Pull request overview

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

Suppressed comments (1)

src/libraries/System.Net.Security/src/System/Net/Security/SslAuthenticationOptions.cs:166

• When UpdateOptions is called repeatedly (e.g., via ServerOptionDelegate for multiple ClientHello frames), the ServerCertificate path can replace an existing owned CertificateContext without releasing the old one. This leaks intermediate cert handles/resources for each update where OwnsCertificateContext was true.

Consider capturing the previous owned context and calling ReleaseResources() after successfully creating/assigning the new context in the ServerCertificate branch, similar to what was added for the ServerCertificateContext branch.

 OwnsCertificateContext = false;
}
else if (sslServerAuthenticationOptions.ServerCertificate != null)
{
X509Certificate2? certificateWithKey = sslServerAuthenticationOptions.ServerCertificate as X509Certificate2;

[azure-pipelines]

Azure Pipelines:
Successfully started running 4 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[wfurt]

LGTM

[Copilot]

SslAuthenticationOptions.UpdateOptions failed to reset OwnsCertificateContext = false when a caller-provided ServerCertificateContext replaced a previously self-owned context. If UpdateOptions was called twice on the same instance — first with only a ServerCertificate (setting OwnsCertificateContext = true), then with a caller-owned ServerCertificateContext — subsequent Dispose() would call ReleaseResources() on the caller's context, disposing its intermediate certificates and breaking cached contexts for future requests.

Changes

• SslAuthenticationOptions.cs — In the ServerCertificateContext != null branch of UpdateOptions:

  • Release the previously-owned context (if any) to avoid resource leak
  • Set OwnsCertificateContext = false so Dispose() won't touch the caller's context

• SslAuthenticationOptionsTests.cs — Regression test using ECDsa P-256 certs that:

  • Simulates two UpdateOptions calls (bare cert → caller context)
  • Asserts OwnsCertificateContext == false and the correct context is set after the second call
  • Verifies the previously-owned context's intermediate is disposed (Handle == IntPtr.Zero)
  • Verifies the caller's intermediates survive Dispose() via Export()

• FixesSslAuthenticationOptions Incorrectly Disposes Unowned IntermediateCertificates #131748

[Copilot]

Copilot was unable to review this pull request because there is no eligible user to bill. To allow Copilot reviews on bot-authored pull requests, enable direct organization billing in your organization's Copilot settings.

[azure-pipelines]

Azure Pipelines:
16 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

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

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/libraries/System.Net.Security/tests/UnitTests/SslAuthenticationOptionsTests.cs:222

• The post-Dispose validation is weak (Subject may still succeed even if the cert handle was freed). Consider asserting via an operation that requires the native handle (e.g., Export) and ensure the test releases the callerContext intermediates to avoid leaking handles across the test run (use ReleaseResources in a finally).

// Verify that the caller's intermediate certificates were not disposed
Assert.True(callerContext.IntermediateCertificates.Count > 0, "Expected at least one intermediate in the caller's context.");

[Copilot]

Pull request overview

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

Suppressed comments (2)

src/libraries/System.Net.Security/src/System/Net/Security/SslAuthenticationOptions.cs:158

• When switching to a caller-provided ServerCertificateContext, UpdateOptions unconditionally releases the previously owned CertificateContext. If the caller passes the same context instance that is currently in CertificateContext (possible in internal flows), this will dispose the intermediates of the context you’re about to keep, potentially breaking the handshake. TlsSession.SetSessionCertificateContext guards against this with a ReferenceEquals check; UpdateOptions should do the same.

 // Release any previously owned context before replacing it with the caller's context.
if (OwnsCertificateContext && CertificateContext != null)
{
CertificateContext.ReleaseResources();

src/libraries/System.Net.Security/tests/UnitTests/SslAuthenticationOptionsTests.cs:202

• This test’s caller-owned context is built with only the intermediate in additionalCertificates. On platforms where chain building requires a trusted root (see SslStreamCertificateContext.Create’s ChainBuildNeedsTrustedRoot retry), chain.Build can yield zero elements on failure and IntermediateCertificates can end up empty, making the test platform-dependent. Including the root certificate in additionalCertificates makes the chain construction deterministic across platforms.

 // Create a caller-owned context with an intermediate certificate
SslStreamCertificateContext callerContext = SslStreamCertificateContext.Create(
leafWithKey,
new X509Certificate2Collection { intermediateWithKey },
offline: true);

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/libraries/System.Net.Security/tests/UnitTests/SslAuthenticationOptionsTests.cs:235

• callerContext holds native crypto handles and cloned certificates (e.g., on Linux SafeX509Handle/SafeEvpPKeyHandle). The test verifies it is not disposed, but then leaves it alive until finalization, which can create unnecessary resource pressure across the test run. After the assertions, explicitly release the context resources.

 foreach (X509Certificate2 cert in callerContext.IntermediateCertificates)
{
Assert.Null(Record.Exception(() => cert.Export(X509ContentType.Cert)));
}

[Copilot]

Pull request overview

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

Suppressed comments (2)

src/libraries/System.Net.Security/src/System/Net/Security/SslAuthenticationOptions.cs:161

• When switching to a provided ServerCertificateContext, the code releases resources based solely on the current ownership bit. If the new context instance happens to be the same object as the current CertificateContext, this will dispose intermediates on the context that will continue to be used, leaving CertificateContext pointing at a released object. Guard the release with a ReferenceEquals check (similar to TlsSession.SetSessionCertificateContext).

 // Release any previously owned context before replacing it with the caller's context.
if (OwnsCertificateContext && CertificateContext is not null &&
!ReferenceEquals(CertificateContext, sslServerAuthenticationOptions.ServerCertificateContext))
{
CertificateContext.ReleaseResources();
}
CertificateContext = sslServerAuthenticationOptions.ServerCertificateContext;

src/libraries/System.Net.Security/tests/UnitTests/SslAuthenticationOptionsTests.cs:206

• ownedIntermediate is declared as non-nullable X509Certificate2 but is assigned null when the intermediate list is empty; with Nullable enabled in this test project, this will produce warnings (and may fail the build if treated as errors). Make the variable nullable.

 Assert.NotEmpty(ownedContext.IntermediateCertificates);
X509Certificate2 ownedIntermediate = ownedContext.IntermediateCertificates[0];
options.CertificateContext = ownedContext;

[Copilot]

Pull request overview

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

Suppressed comments (1)

src/libraries/System.Net.Security/src/System/Net/Security/SslAuthenticationOptions.cs:166

• When UpdateOptions is called repeatedly (e.g., via ServerOptionDelegate for multiple ClientHello frames), the ServerCertificate path can replace an existing owned CertificateContext without releasing the old one. This leaks intermediate cert handles/resources for each update where OwnsCertificateContext was true.

Consider capturing the previous owned context and calling ReleaseResources() after successfully creating/assigning the new context in the ServerCertificate branch, similar to what was added for the ServerCertificateContext branch.

 OwnsCertificateContext = false;
}
else if (sslServerAuthenticationOptions.ServerCertificate != null)
{
X509Certificate2? certificateWithKey = sslServerAuthenticationOptions.ServerCertificate as X509Certificate2;

[azure-pipelines]

Azure Pipelines:
Successfully started running 4 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[wfurt]

LGTM

[Copilot]

SslAuthenticationOptions.UpdateOptions failed to reset OwnsCertificateContext = false when a caller-provided ServerCertificateContext replaced a previously self-owned context. If UpdateOptions was called twice on the same instance — first with only a ServerCertificate (setting OwnsCertificateContext = true), then with a caller-owned ServerCertificateContext — subsequent Dispose() would call ReleaseResources() on the caller's context, disposing its intermediate certificates and breaking cached contexts for future requests.

Changes

• SslAuthenticationOptions.cs — In the ServerCertificateContext != null branch of UpdateOptions:

  • Release the previously-owned context (if any) to avoid resource leak
  • Set OwnsCertificateContext = false so Dispose() won't touch the caller's context

• SslAuthenticationOptionsTests.cs — Regression test using ECDsa P-256 certs that:

  • Simulates two UpdateOptions calls (bare cert → caller context)
  • Asserts OwnsCertificateContext == false and the correct context is set after the second call
  • Verifies the previously-owned context's intermediate is disposed (Handle == IntPtr.Zero)
  • Verifies the caller's intermediates survive Dispose() via Export()

• FixesSslAuthenticationOptions Incorrectly Disposes Unowned IntermediateCertificates #131748

[Copilot]

Copilot was unable to review this pull request because there is no eligible user to bill. To allow Copilot reviews on bot-authored pull requests, enable direct organization billing in your organization's Copilot settings.

[azure-pipelines]

Azure Pipelines:
16 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

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

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/libraries/System.Net.Security/tests/UnitTests/SslAuthenticationOptionsTests.cs:222

• The post-Dispose validation is weak (Subject may still succeed even if the cert handle was freed). Consider asserting via an operation that requires the native handle (e.g., Export) and ensure the test releases the callerContext intermediates to avoid leaking handles across the test run (use ReleaseResources in a finally).

// Verify that the caller's intermediate certificates were not disposed
Assert.True(callerContext.IntermediateCertificates.Count > 0, "Expected at least one intermediate in the caller's context.");

[Copilot]

Pull request overview

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

Suppressed comments (2)

src/libraries/System.Net.Security/src/System/Net/Security/SslAuthenticationOptions.cs:158

• When switching to a caller-provided ServerCertificateContext, UpdateOptions unconditionally releases the previously owned CertificateContext. If the caller passes the same context instance that is currently in CertificateContext (possible in internal flows), this will dispose the intermediates of the context you’re about to keep, potentially breaking the handshake. TlsSession.SetSessionCertificateContext guards against this with a ReferenceEquals check; UpdateOptions should do the same.

 // Release any previously owned context before replacing it with the caller's context.
if (OwnsCertificateContext && CertificateContext != null)
{
CertificateContext.ReleaseResources();

src/libraries/System.Net.Security/tests/UnitTests/SslAuthenticationOptionsTests.cs:202

• This test’s caller-owned context is built with only the intermediate in additionalCertificates. On platforms where chain building requires a trusted root (see SslStreamCertificateContext.Create’s ChainBuildNeedsTrustedRoot retry), chain.Build can yield zero elements on failure and IntermediateCertificates can end up empty, making the test platform-dependent. Including the root certificate in additionalCertificates makes the chain construction deterministic across platforms.

 // Create a caller-owned context with an intermediate certificate
SslStreamCertificateContext callerContext = SslStreamCertificateContext.Create(
leafWithKey,
new X509Certificate2Collection { intermediateWithKey },
offline: true);

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/libraries/System.Net.Security/tests/UnitTests/SslAuthenticationOptionsTests.cs:235

• callerContext holds native crypto handles and cloned certificates (e.g., on Linux SafeX509Handle/SafeEvpPKeyHandle). The test verifies it is not disposed, but then leaves it alive until finalization, which can create unnecessary resource pressure across the test run. After the assertions, explicitly release the context resources.

 foreach (X509Certificate2 cert in callerContext.IntermediateCertificates)
{
Assert.Null(Record.Exception(() => cert.Export(X509ContentType.Cert)));
}

[Copilot]

Pull request overview

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

Suppressed comments (2)

src/libraries/System.Net.Security/src/System/Net/Security/SslAuthenticationOptions.cs:161

• When switching to a provided ServerCertificateContext, the code releases resources based solely on the current ownership bit. If the new context instance happens to be the same object as the current CertificateContext, this will dispose intermediates on the context that will continue to be used, leaving CertificateContext pointing at a released object. Guard the release with a ReferenceEquals check (similar to TlsSession.SetSessionCertificateContext).

 // Release any previously owned context before replacing it with the caller's context.
if (OwnsCertificateContext && CertificateContext is not null &&
!ReferenceEquals(CertificateContext, sslServerAuthenticationOptions.ServerCertificateContext))
{
CertificateContext.ReleaseResources();
}
CertificateContext = sslServerAuthenticationOptions.ServerCertificateContext;

src/libraries/System.Net.Security/tests/UnitTests/SslAuthenticationOptionsTests.cs:206

• ownedIntermediate is declared as non-nullable X509Certificate2 but is assigned null when the intermediate list is empty; with Nullable enabled in this test project, this will produce warnings (and may fail the build if treated as errors). Make the variable nullable.

 Assert.NotEmpty(ownedContext.IntermediateCertificates);
X509Certificate2 ownedIntermediate = ownedContext.IntermediateCertificates[0];
options.CertificateContext = ownedContext;

[Copilot]

Pull request overview

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

Suppressed comments (1)

src/libraries/System.Net.Security/src/System/Net/Security/SslAuthenticationOptions.cs:166

• When UpdateOptions is called repeatedly (e.g., via ServerOptionDelegate for multiple ClientHello frames), the ServerCertificate path can replace an existing owned CertificateContext without releasing the old one. This leaks intermediate cert handles/resources for each update where OwnsCertificateContext was true.

Consider capturing the previous owned context and calling ReleaseResources() after successfully creating/assigning the new context in the ServerCertificate branch, similar to what was added for the ServerCertificateContext branch.

 OwnsCertificateContext = false;
}
else if (sslServerAuthenticationOptions.ServerCertificate != null)
{
X509Certificate2? certificateWithKey = sslServerAuthenticationOptions.ServerCertificate as X509Certificate2;

[azure-pipelines]

Azure Pipelines:
Successfully started running 4 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[wfurt]

LGTM

[Copilot]

SslAuthenticationOptions.UpdateOptions failed to reset OwnsCertificateContext = false when a caller-provided ServerCertificateContext replaced a previously self-owned context. If UpdateOptions was called twice on the same instance — first with only a ServerCertificate (setting OwnsCertificateContext = true), then with a caller-owned ServerCertificateContext — subsequent Dispose() would call ReleaseResources() on the caller's context, disposing its intermediate certificates and breaking cached contexts for future requests.

Changes

• SslAuthenticationOptions.cs — In the ServerCertificateContext != null branch of UpdateOptions:

  • Release the previously-owned context (if any) to avoid resource leak
  • Set OwnsCertificateContext = false so Dispose() won't touch the caller's context

• SslAuthenticationOptionsTests.cs — Regression test using ECDsa P-256 certs that:

  • Simulates two UpdateOptions calls (bare cert → caller context)
  • Asserts OwnsCertificateContext == false and the correct context is set after the second call
  • Verifies the previously-owned context's intermediate is disposed (Handle == IntPtr.Zero)
  • Verifies the caller's intermediates survive Dispose() via Export()

• FixesSslAuthenticationOptions Incorrectly Disposes Unowned IntermediateCertificates #131748

[Copilot]

Copilot was unable to review this pull request because there is no eligible user to bill. To allow Copilot reviews on bot-authored pull requests, enable direct organization billing in your organization's Copilot settings.

[azure-pipelines]

Azure Pipelines:
16 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

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

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/libraries/System.Net.Security/tests/UnitTests/SslAuthenticationOptionsTests.cs:222

• The post-Dispose validation is weak (Subject may still succeed even if the cert handle was freed). Consider asserting via an operation that requires the native handle (e.g., Export) and ensure the test releases the callerContext intermediates to avoid leaking handles across the test run (use ReleaseResources in a finally).

// Verify that the caller's intermediate certificates were not disposed
Assert.True(callerContext.IntermediateCertificates.Count > 0, "Expected at least one intermediate in the caller's context.");

[Copilot]

Pull request overview

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

Suppressed comments (2)

src/libraries/System.Net.Security/src/System/Net/Security/SslAuthenticationOptions.cs:158

• When switching to a caller-provided ServerCertificateContext, UpdateOptions unconditionally releases the previously owned CertificateContext. If the caller passes the same context instance that is currently in CertificateContext (possible in internal flows), this will dispose the intermediates of the context you’re about to keep, potentially breaking the handshake. TlsSession.SetSessionCertificateContext guards against this with a ReferenceEquals check; UpdateOptions should do the same.

 // Release any previously owned context before replacing it with the caller's context.
if (OwnsCertificateContext && CertificateContext != null)
{
CertificateContext.ReleaseResources();

src/libraries/System.Net.Security/tests/UnitTests/SslAuthenticationOptionsTests.cs:202

• This test’s caller-owned context is built with only the intermediate in additionalCertificates. On platforms where chain building requires a trusted root (see SslStreamCertificateContext.Create’s ChainBuildNeedsTrustedRoot retry), chain.Build can yield zero elements on failure and IntermediateCertificates can end up empty, making the test platform-dependent. Including the root certificate in additionalCertificates makes the chain construction deterministic across platforms.

 // Create a caller-owned context with an intermediate certificate
SslStreamCertificateContext callerContext = SslStreamCertificateContext.Create(
leafWithKey,
new X509Certificate2Collection { intermediateWithKey },
offline: true);

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/libraries/System.Net.Security/tests/UnitTests/SslAuthenticationOptionsTests.cs:235

• callerContext holds native crypto handles and cloned certificates (e.g., on Linux SafeX509Handle/SafeEvpPKeyHandle). The test verifies it is not disposed, but then leaves it alive until finalization, which can create unnecessary resource pressure across the test run. After the assertions, explicitly release the context resources.

 foreach (X509Certificate2 cert in callerContext.IntermediateCertificates)
{
Assert.Null(Record.Exception(() => cert.Export(X509ContentType.Cert)));
}

[Copilot]

Pull request overview

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

Suppressed comments (2)

src/libraries/System.Net.Security/src/System/Net/Security/SslAuthenticationOptions.cs:161

• When switching to a provided ServerCertificateContext, the code releases resources based solely on the current ownership bit. If the new context instance happens to be the same object as the current CertificateContext, this will dispose intermediates on the context that will continue to be used, leaving CertificateContext pointing at a released object. Guard the release with a ReferenceEquals check (similar to TlsSession.SetSessionCertificateContext).

 // Release any previously owned context before replacing it with the caller's context.
if (OwnsCertificateContext && CertificateContext is not null &&
!ReferenceEquals(CertificateContext, sslServerAuthenticationOptions.ServerCertificateContext))
{
CertificateContext.ReleaseResources();
}
CertificateContext = sslServerAuthenticationOptions.ServerCertificateContext;

src/libraries/System.Net.Security/tests/UnitTests/SslAuthenticationOptionsTests.cs:206

• ownedIntermediate is declared as non-nullable X509Certificate2 but is assigned null when the intermediate list is empty; with Nullable enabled in this test project, this will produce warnings (and may fail the build if treated as errors). Make the variable nullable.

 Assert.NotEmpty(ownedContext.IntermediateCertificates);
X509Certificate2 ownedIntermediate = ownedContext.IntermediateCertificates[0];
options.CertificateContext = ownedContext;

[Copilot]

Pull request overview

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

Suppressed comments (1)

src/libraries/System.Net.Security/src/System/Net/Security/SslAuthenticationOptions.cs:166

• When UpdateOptions is called repeatedly (e.g., via ServerOptionDelegate for multiple ClientHello frames), the ServerCertificate path can replace an existing owned CertificateContext without releasing the old one. This leaks intermediate cert handles/resources for each update where OwnsCertificateContext was true.

Consider capturing the previous owned context and calling ReleaseResources() after successfully creating/assigning the new context in the ServerCertificate branch, similar to what was added for the ServerCertificateContext branch.

 OwnsCertificateContext = false;
}
else if (sslServerAuthenticationOptions.ServerCertificate != null)
{
X509Certificate2? certificateWithKey = sslServerAuthenticationOptions.ServerCertificate as X509Certificate2;

[azure-pipelines]

Azure Pipelines:
Successfully started running 4 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[wfurt]

LGTM

[Copilot]

SslAuthenticationOptions.UpdateOptions failed to reset OwnsCertificateContext = false when a caller-provided ServerCertificateContext replaced a previously self-owned context. If UpdateOptions was called twice on the same instance — first with only a ServerCertificate (setting OwnsCertificateContext = true), then with a caller-owned ServerCertificateContext — subsequent Dispose() would call ReleaseResources() on the caller's context, disposing its intermediate certificates and breaking cached contexts for future requests.

Changes

• SslAuthenticationOptions.cs — In the ServerCertificateContext != null branch of UpdateOptions:

  • Release the previously-owned context (if any) to avoid resource leak
  • Set OwnsCertificateContext = false so Dispose() won't touch the caller's context

• SslAuthenticationOptionsTests.cs — Regression test using ECDsa P-256 certs that:

  • Simulates two UpdateOptions calls (bare cert → caller context)
  • Asserts OwnsCertificateContext == false and the correct context is set after the second call
  • Verifies the previously-owned context's intermediate is disposed (Handle == IntPtr.Zero)
  • Verifies the caller's intermediates survive Dispose() via Export()

• FixesSslAuthenticationOptions Incorrectly Disposes Unowned IntermediateCertificates #131748

[Copilot]

Copilot was unable to review this pull request because there is no eligible user to bill. To allow Copilot reviews on bot-authored pull requests, enable direct organization billing in your organization's Copilot settings.

[azure-pipelines]

Azure Pipelines:
16 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

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

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/libraries/System.Net.Security/tests/UnitTests/SslAuthenticationOptionsTests.cs:222

• The post-Dispose validation is weak (Subject may still succeed even if the cert handle was freed). Consider asserting via an operation that requires the native handle (e.g., Export) and ensure the test releases the callerContext intermediates to avoid leaking handles across the test run (use ReleaseResources in a finally).

// Verify that the caller's intermediate certificates were not disposed
Assert.True(callerContext.IntermediateCertificates.Count > 0, "Expected at least one intermediate in the caller's context.");

[Copilot]

Pull request overview

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

Suppressed comments (2)

src/libraries/System.Net.Security/src/System/Net/Security/SslAuthenticationOptions.cs:158

• When switching to a caller-provided ServerCertificateContext, UpdateOptions unconditionally releases the previously owned CertificateContext. If the caller passes the same context instance that is currently in CertificateContext (possible in internal flows), this will dispose the intermediates of the context you’re about to keep, potentially breaking the handshake. TlsSession.SetSessionCertificateContext guards against this with a ReferenceEquals check; UpdateOptions should do the same.

 // Release any previously owned context before replacing it with the caller's context.
if (OwnsCertificateContext && CertificateContext != null)
{
CertificateContext.ReleaseResources();

src/libraries/System.Net.Security/tests/UnitTests/SslAuthenticationOptionsTests.cs:202

• This test’s caller-owned context is built with only the intermediate in additionalCertificates. On platforms where chain building requires a trusted root (see SslStreamCertificateContext.Create’s ChainBuildNeedsTrustedRoot retry), chain.Build can yield zero elements on failure and IntermediateCertificates can end up empty, making the test platform-dependent. Including the root certificate in additionalCertificates makes the chain construction deterministic across platforms.

 // Create a caller-owned context with an intermediate certificate
SslStreamCertificateContext callerContext = SslStreamCertificateContext.Create(
leafWithKey,
new X509Certificate2Collection { intermediateWithKey },
offline: true);

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/libraries/System.Net.Security/tests/UnitTests/SslAuthenticationOptionsTests.cs:235

• callerContext holds native crypto handles and cloned certificates (e.g., on Linux SafeX509Handle/SafeEvpPKeyHandle). The test verifies it is not disposed, but then leaves it alive until finalization, which can create unnecessary resource pressure across the test run. After the assertions, explicitly release the context resources.

 foreach (X509Certificate2 cert in callerContext.IntermediateCertificates)
{
Assert.Null(Record.Exception(() => cert.Export(X509ContentType.Cert)));
}

[Copilot]

Pull request overview

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

Suppressed comments (2)

src/libraries/System.Net.Security/src/System/Net/Security/SslAuthenticationOptions.cs:161

• When switching to a provided ServerCertificateContext, the code releases resources based solely on the current ownership bit. If the new context instance happens to be the same object as the current CertificateContext, this will dispose intermediates on the context that will continue to be used, leaving CertificateContext pointing at a released object. Guard the release with a ReferenceEquals check (similar to TlsSession.SetSessionCertificateContext).

 // Release any previously owned context before replacing it with the caller's context.
if (OwnsCertificateContext && CertificateContext is not null &&
!ReferenceEquals(CertificateContext, sslServerAuthenticationOptions.ServerCertificateContext))
{
CertificateContext.ReleaseResources();
}
CertificateContext = sslServerAuthenticationOptions.ServerCertificateContext;

src/libraries/System.Net.Security/tests/UnitTests/SslAuthenticationOptionsTests.cs:206

• ownedIntermediate is declared as non-nullable X509Certificate2 but is assigned null when the intermediate list is empty; with Nullable enabled in this test project, this will produce warnings (and may fail the build if treated as errors). Make the variable nullable.

 Assert.NotEmpty(ownedContext.IntermediateCertificates);
X509Certificate2 ownedIntermediate = ownedContext.IntermediateCertificates[0];
options.CertificateContext = ownedContext;

[Copilot]

Pull request overview

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

Suppressed comments (1)

src/libraries/System.Net.Security/src/System/Net/Security/SslAuthenticationOptions.cs:166

• When UpdateOptions is called repeatedly (e.g., via ServerOptionDelegate for multiple ClientHello frames), the ServerCertificate path can replace an existing owned CertificateContext without releasing the old one. This leaks intermediate cert handles/resources for each update where OwnsCertificateContext was true.

Consider capturing the previous owned context and calling ReleaseResources() after successfully creating/assigning the new context in the ServerCertificate branch, similar to what was added for the ServerCertificateContext branch.

 OwnsCertificateContext = false;
}
else if (sslServerAuthenticationOptions.ServerCertificate != null)
{
X509Certificate2? certificateWithKey = sslServerAuthenticationOptions.ServerCertificate as X509Certificate2;

[azure-pipelines]

Azure Pipelines:
Successfully started running 4 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[wfurt]

LGTM