From 49e9272b881b1f12c2064982c6ccaf57aa00151a Mon Sep 17 00:00:00 2001 From: jashook Date: Tue, 1 Jul 2025 14:56:09 -0700 Subject: [PATCH 1/3] Dispose the certificate chain elements with the chain --- .../src/CertificateAuthenticationHandler.cs | 30 ++++++++++++----- .../UnixCertificateManager.cs | 33 +++++++++++++------ 2 files changed, 45 insertions(+), 18 deletions(-) diff --git a/src/Security/Authentication/Certificate/src/CertificateAuthenticationHandler.cs b/src/Security/Authentication/Certificate/src/CertificateAuthenticationHandler.cs index 8f8873057027..93c30cf0f733 100644 --- a/src/Security/Authentication/Certificate/src/CertificateAuthenticationHandler.cs +++ b/src/Security/Authentication/Certificate/src/CertificateAuthenticationHandler.cs @@ -135,21 +135,35 @@ private async Task ValidateCertificateAsync(X509Certificate2 } var chainPolicy = BuildChainPolicy(clientCertificate, isCertificateSelfSigned); - using var chain = new X509Chain + var chain = new X509Chain { ChainPolicy = chainPolicy }; - var certificateIsValid = chain.Build(clientCertificate); - if (!certificateIsValid) + try + { + var certificateIsValid = chain.Build(clientCertificate); + if (!certificateIsValid) + { + var chainErrors = new List(chain.ChainStatus.Length); + foreach (var validationFailure in chain.ChainStatus) + { + chainErrors.Add($"{validationFailure.Status} {validationFailure.StatusInformation}"); + } + Logger.CertificateFailedValidation(clientCertificate.Subject, chainErrors); + return AuthenticateResults.InvalidClientCertificate; + } + } + finally { - var chainErrors = new List(chain.ChainStatus.Length); - foreach (var validationFailure in chain.ChainStatus) + // Disposing the chain does not dispose the elements we potentially built. + // Annoyingly do the full walk manually to dispose. + for (int chainElementIndex = 0; chainElementIndex < chain.ChainElements.Count; ++chainElementIndex) { - chainErrors.Add($"{validationFailure.Status} {validationFailure.StatusInformation}"); + chain.ChainElements[chainElementIndex].Certificate.Dispose(); } - Logger.CertificateFailedValidation(clientCertificate.Subject, chainErrors); - return AuthenticateResults.InvalidClientCertificate; + + chain.Dispose(); } var certificateValidatedContext = new CertificateValidatedContext(Context, Scheme, Options) diff --git a/src/Shared/CertificateGeneration/UnixCertificateManager.cs b/src/Shared/CertificateGeneration/UnixCertificateManager.cs index 149e0fab3ba6..9d06b29f3399 100644 --- a/src/Shared/CertificateGeneration/UnixCertificateManager.cs +++ b/src/Shared/CertificateGeneration/UnixCertificateManager.cs @@ -62,18 +62,31 @@ public override TrustLevel GetTrustLevel(X509Certificate2 certificate) // Building the chain will check whether dotnet trusts the cert. We could, instead, // enumerate the Root store and/or look for the file in the OpenSSL directory, but // this tests the real-world behavior. - using var chain = new X509Chain(); - // This is just a heuristic for whether or not we should prompt the user to re-run with `--trust` - // so we don't need to check revocation (which doesn't really make sense for dev certs anyway) - chain.ChainPolicy.RevocationMode = X509RevocationMode.NoCheck; - if (chain.Build(certificate)) + var chain = new X509Chain(); + try { - sawTrustSuccess = true; - } - else + // This is just a heuristic for whether or not we should prompt the user to re-run with `--trust` + // so we don't need to check revocation (which doesn't really make sense for dev certs anyway) + chain.ChainPolicy.RevocationMode = X509RevocationMode.NoCheck; + if (chain.Build(certificate)) + { + sawTrustSuccess = true; + } + else + { + sawTrustFailure = true; + Log.UnixNotTrustedByDotnet(); + } + finally { - sawTrustFailure = true; - Log.UnixNotTrustedByDotnet(); + // Disposing the chain does not dispose the elements we potentially built. + // Annoyingly do the full walk manually to dispose. + for (int chainElementIndex = 0; chainElementIndex < chain.ChainElements.Count; ++chainElementIndex) + { + chain.ChainElements[chainElementIndex].Certificate.Dispose(); + } + + chain.Dispose(); } // Will become the name of the file on disk and the nickname in the NSS DBs From 9672f746ef93cc86ac76e5cccee8f3c10033390e Mon Sep 17 00:00:00 2001 From: jashook Date: Tue, 1 Jul 2025 15:00:08 -0700 Subject: [PATCH 2/3] Fix the missing brace --- src/Shared/CertificateGeneration/UnixCertificateManager.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Shared/CertificateGeneration/UnixCertificateManager.cs b/src/Shared/CertificateGeneration/UnixCertificateManager.cs index 9d06b29f3399..7fb5b2f96f44 100644 --- a/src/Shared/CertificateGeneration/UnixCertificateManager.cs +++ b/src/Shared/CertificateGeneration/UnixCertificateManager.cs @@ -77,6 +77,7 @@ public override TrustLevel GetTrustLevel(X509Certificate2 certificate) sawTrustFailure = true; Log.UnixNotTrustedByDotnet(); } + } finally { // Disposing the chain does not dispose the elements we potentially built. From cf7235c3a0b8fc5a841dd4838fdea4975b5d3557 Mon Sep 17 00:00:00 2001 From: jashook Date: Tue, 1 Jul 2025 17:42:06 -0700 Subject: [PATCH 3/3] Remove snarky comment. --- .../Certificate/src/CertificateAuthenticationHandler.cs | 2 +- src/Shared/CertificateGeneration/UnixCertificateManager.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Security/Authentication/Certificate/src/CertificateAuthenticationHandler.cs b/src/Security/Authentication/Certificate/src/CertificateAuthenticationHandler.cs index 93c30cf0f733..c948d3c3a74d 100644 --- a/src/Security/Authentication/Certificate/src/CertificateAuthenticationHandler.cs +++ b/src/Security/Authentication/Certificate/src/CertificateAuthenticationHandler.cs @@ -157,7 +157,7 @@ private async Task ValidateCertificateAsync(X509Certificate2 finally { // Disposing the chain does not dispose the elements we potentially built. - // Annoyingly do the full walk manually to dispose. + // Do the full walk manually to dispose. for (int chainElementIndex = 0; chainElementIndex < chain.ChainElements.Count; ++chainElementIndex) { chain.ChainElements[chainElementIndex].Certificate.Dispose(); diff --git a/src/Shared/CertificateGeneration/UnixCertificateManager.cs b/src/Shared/CertificateGeneration/UnixCertificateManager.cs index 7fb5b2f96f44..4577617a1def 100644 --- a/src/Shared/CertificateGeneration/UnixCertificateManager.cs +++ b/src/Shared/CertificateGeneration/UnixCertificateManager.cs @@ -81,7 +81,7 @@ public override TrustLevel GetTrustLevel(X509Certificate2 certificate) finally { // Disposing the chain does not dispose the elements we potentially built. - // Annoyingly do the full walk manually to dispose. + // Do the full walk manually to dispose. for (int chainElementIndex = 0; chainElementIndex < chain.ChainElements.Count; ++chainElementIndex) { chain.ChainElements[chainElementIndex].Certificate.Dispose();