From 87e23cab685ee3b3bf76ccf71cd4ad7ddea8a3bb Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Wed, 18 Nov 2020 14:10:48 -0500 Subject: [PATCH] Avoid expensive work for CertificateManager logging when not enabled --- .../CertificateManager.cs | 144 +++++++++++++----- .../MacOSCertificateManager.cs | 17 ++- 2 files changed, 120 insertions(+), 41 deletions(-) diff --git a/src/Shared/CertificateGeneration/CertificateManager.cs b/src/Shared/CertificateGeneration/CertificateManager.cs index 8ad2e14749f2..d767db8b8f81 100644 --- a/src/Shared/CertificateGeneration/CertificateManager.cs +++ b/src/Shared/CertificateGeneration/CertificateManager.cs @@ -80,7 +80,10 @@ public IList ListCertificates( matchingCertificates = matchingCertificates .Where(c => HasOid(c, AspNetHttpsOid)); - Log.DescribeFoundCertificates(ToCertificateDescription(matchingCertificates)); + if (Log.IsEnabled()) + { + Log.DescribeFoundCertificates(ToCertificateDescription(matchingCertificates)); + } if (isValid) { @@ -93,10 +96,12 @@ public IList ListCertificates( .OrderByDescending(c => GetCertificateVersion(c)) .ToArray(); - var invalidCertificates = matchingCertificates.Except(validCertificates); - - Log.DescribeValidCertificates(ToCertificateDescription(validCertificates)); - Log.DescribeInvalidValidCertificates(ToCertificateDescription(invalidCertificates)); + if (Log.IsEnabled()) + { + var invalidCertificates = matchingCertificates.Except(validCertificates); + Log.DescribeValidCertificates(ToCertificateDescription(validCertificates)); + Log.DescribeInvalidValidCertificates(ToCertificateDescription(invalidCertificates)); + } matchingCertificates = validCertificates; } @@ -114,7 +119,10 @@ public IList ListCertificates( } catch (Exception e) { - Log.ListCertificatesError(e.ToString()); + if (Log.IsEnabled()) + { + Log.ListCertificatesError(e.ToString()); + } DisposeCertificates(certificates); certificates.Clear(); return certificates; @@ -170,10 +178,13 @@ public EnsureCertificateResult EnsureAspNetCoreHttpsDevelopmentCertificate( var certificates = currentUserCertificates.Concat(trustedCertificates); var filteredCertificates = certificates.Where(c => c.Subject == Subject); - var excludedCertificates = certificates.Except(filteredCertificates); - Log.FilteredCertificates(ToCertificateDescription(filteredCertificates)); - Log.ExcludedCertificates(ToCertificateDescription(excludedCertificates)); + if (Log.IsEnabled()) + { + var excludedCertificates = certificates.Except(filteredCertificates); + Log.FilteredCertificates(ToCertificateDescription(filteredCertificates)); + Log.ExcludedCertificates(ToCertificateDescription(excludedCertificates)); + } certificates = filteredCertificates; @@ -194,13 +205,19 @@ public EnsureCertificateResult EnsureAspNetCoreHttpsDevelopmentCertificate( { try { - Log.CorrectCertificateStateStart(GetDescription(candidate)); + if (Log.IsEnabled()) + { + Log.CorrectCertificateStateStart(GetDescription(candidate)); + } CorrectCertificateState(candidate); Log.CorrectCertificateStateEnd(); } catch (Exception e) { - Log.CorrectCertificateStateError(e.ToString()); + if (Log.IsEnabled()) + { + Log.CorrectCertificateStateError(e.ToString()); + } result = EnsureCertificateResult.FailedToMakeKeyAccessible; // We don't return early on this type of failure to allow for tooling to // export or trust the certificate even in this situation, as that enables @@ -213,9 +230,15 @@ public EnsureCertificateResult EnsureAspNetCoreHttpsDevelopmentCertificate( if (!failedToFixCertificateState) { - Log.ValidCertificatesFound(ToCertificateDescription(certificates)); + if (Log.IsEnabled()) + { + Log.ValidCertificatesFound(ToCertificateDescription(certificates)); + } certificate = certificates.First(); - Log.SelectedCertificate(GetDescription(certificate)); + if (Log.IsEnabled()) + { + Log.SelectedCertificate(GetDescription(certificate)); + } result = EnsureCertificateResult.ValidCertificatePresent; } } @@ -230,7 +253,10 @@ public EnsureCertificateResult EnsureAspNetCoreHttpsDevelopmentCertificate( } catch (Exception e) { - Log.CreateDevelopmentCertificateError(e.ToString()); + if (Log.IsEnabled()) + { + Log.CreateDevelopmentCertificateError(e.ToString()); + } result = EnsureCertificateResult.ErrorCreatingTheCertificate; return result; } @@ -251,13 +277,20 @@ public EnsureCertificateResult EnsureAspNetCoreHttpsDevelopmentCertificate( { try { - Log.CorrectCertificateStateStart(GetDescription(certificate)); + if (Log.IsEnabled()) + { + Log.CorrectCertificateStateStart(GetDescription(certificate)); + } CorrectCertificateState(certificate); Log.CorrectCertificateStateEnd(); } catch (Exception e) { - Log.CorrectCertificateStateError(e.ToString()); + if (Log.IsEnabled()) + { + Log.CorrectCertificateStateError(e.ToString()); + } + // We don't return early on this type of failure to allow for tooling to // export or trust the certificate even in this situation, as that enables // exporting the certificate to perform any necessary fix with native tooling. @@ -274,7 +307,11 @@ public EnsureCertificateResult EnsureAspNetCoreHttpsDevelopmentCertificate( } catch (Exception e) { - Log.ExportCertificateError(e.ToString()); + if (Log.IsEnabled()) + { + Log.ExportCertificateError(e.ToString()); + } + // We don't want to mask the original source of the error here. result = result != EnsureCertificateResult.Succeeded && result != EnsureCertificateResult.ValidCertificatePresent ? result : @@ -318,7 +355,10 @@ internal ImportCertificateResult ImportCertificate(string certificatePath, strin var certificates = ListCertificates(StoreName.My, StoreLocation.CurrentUser, isValid: false, requireExportable: false); if (certificates.Any()) { - Log.ImportCertificateExistingCertificates(ToCertificateDescription(certificates)); + if (Log.IsEnabled()) + { + Log.ImportCertificateExistingCertificates(ToCertificateDescription(certificates)); + } return ImportCertificateResult.ExistingCertificatesPresent; } @@ -327,17 +367,26 @@ internal ImportCertificateResult ImportCertificate(string certificatePath, strin { Log.LoadCertificateStart(certificatePath); certificate = new X509Certificate2(certificatePath, password, X509KeyStorageFlags.Exportable | X509KeyStorageFlags.EphemeralKeySet); - Log.LoadCertificateEnd(GetDescription(certificate)); + if (Log.IsEnabled()) + { + Log.LoadCertificateEnd(GetDescription(certificate)); + } } catch (Exception e) { - Log.LoadCertificateError(e.ToString()); + if (Log.IsEnabled()) + { + Log.LoadCertificateError(e.ToString()); + } return ImportCertificateResult.InvalidCertificate; } if (!IsHttpsDevelopmentCertificate(certificate)) { - Log.NoHttpsDevelopmentCertificate(GetDescription(certificate)); + if (Log.IsEnabled()) + { + Log.NoHttpsDevelopmentCertificate(GetDescription(certificate)); + } return ImportCertificateResult.NoDevelopmentHttpsCertificate; } @@ -347,7 +396,10 @@ internal ImportCertificateResult ImportCertificate(string certificatePath, strin } catch (Exception e) { - Log.SaveCertificateInStoreError(e.ToString()); + if (Log.IsEnabled()) + { + Log.SaveCertificateInStoreError(e.ToString()); + } return ImportCertificateResult.ErrorSavingTheCertificateIntoTheCurrentUserPersonalStore; } @@ -365,10 +417,13 @@ public void CleanupHttpsCertificates() // we remove the certificates from the local user store to finish up the cleanup. var certificates = ListCertificates(StoreName.My, StoreLocation.CurrentUser, isValid: false); var filteredCertificates = certificates.Where(c => c.Subject == Subject); - var excludedCertificates = certificates.Except(filteredCertificates); - Log.FilteredCertificates(ToCertificateDescription(filteredCertificates)); - Log.ExcludedCertificates(ToCertificateDescription(excludedCertificates)); + if (Log.IsEnabled()) + { + var excludedCertificates = certificates.Except(filteredCertificates); + Log.FilteredCertificates(ToCertificateDescription(filteredCertificates)); + Log.ExcludedCertificates(ToCertificateDescription(excludedCertificates)); + } foreach (var certificate in filteredCertificates) { @@ -390,7 +445,11 @@ public void CleanupHttpsCertificates() internal void ExportCertificate(X509Certificate2 certificate, string path, bool includePrivateKey, string password, CertificateKeyExportFormat format) { - Log.ExportCertificateStart(GetDescription(certificate), path, includePrivateKey); + if (Log.IsEnabled()) + { + Log.ExportCertificateStart(GetDescription(certificate), path, includePrivateKey); + } + if (includePrivateKey && password == null) { Log.NoPasswordForCertificate(); @@ -465,7 +524,7 @@ internal void ExportCertificate(X509Certificate2 certificate, string path, bool } } } - catch (Exception e) + catch (Exception e) when (Log.IsEnabled()) { Log.ExportCertificateError(e.ToString()); throw; @@ -480,7 +539,7 @@ internal void ExportCertificate(X509Certificate2 certificate, string path, bool Log.WriteCertificateToDisk(path); File.WriteAllBytes(path, bytes); } - catch (Exception ex) + catch (Exception ex) when (Log.IsEnabled()) { Log.WriteCertificateToDiskError(ex.ToString()); throw; @@ -498,7 +557,7 @@ internal void ExportCertificate(X509Certificate2 certificate, string path, bool Log.WritePemKeyToDisk(keyPath); File.WriteAllBytes(keyPath, pemEnvelope); } - catch (Exception ex) + catch (Exception ex) when (Log.IsEnabled()) { Log.WritePemKeyToDiskError(ex.ToString()); throw; @@ -565,7 +624,10 @@ internal X509Certificate2 SaveCertificate(X509Certificate2 certificate) var name = StoreName.My; var location = StoreLocation.CurrentUser; - Log.SaveCertificateInStoreStart(GetDescription(certificate), name, location); + if (Log.IsEnabled()) + { + Log.SaveCertificateInStoreStart(GetDescription(certificate), name, location); + } certificate = SaveCertificateCore(certificate); @@ -577,11 +639,14 @@ internal void TrustCertificate(X509Certificate2 certificate) { try { - Log.TrustCertificateStart(GetDescription(certificate)); + if (Log.IsEnabled()) + { + Log.TrustCertificateStart(GetDescription(certificate)); + } TrustCertificateCore(certificate); Log.TrustCertificateEnd(); } - catch (Exception ex) + catch (Exception ex) when (Log.IsEnabled()) { Log.TrustCertificateError(ex.ToString()); throw; @@ -676,7 +741,10 @@ private static void RemoveCertificateFromUserStore(X509Certificate2 certificate) { try { - Log.RemoveCertificateFromUserStoreStart(GetDescription(certificate)); + if (Log.IsEnabled()) + { + Log.RemoveCertificateFromUserStoreStart(GetDescription(certificate)); + } using var store = new X509Store(StoreName.My, StoreLocation.CurrentUser); store.Open(OpenFlags.ReadWrite); var matching = store.Certificates @@ -687,7 +755,7 @@ private static void RemoveCertificateFromUserStore(X509Certificate2 certificate) store.Close(); Log.RemoveCertificateFromUserStoreEnd(); } - catch (Exception ex) + catch (Exception ex) when (Log.IsEnabled()) { Log.RemoveCertificateFromUserStoreError(ex.ToString()); throw; @@ -695,10 +763,10 @@ private static void RemoveCertificateFromUserStore(X509Certificate2 certificate) } internal static string ToCertificateDescription(IEnumerable matchingCertificates) => - string.Join(Environment.NewLine, matchingCertificates - .OrderBy(c => c.Thumbprint) - .Select(c => GetDescription(c)) - .ToArray()); + string.Join(Environment.NewLine, matchingCertificates + .OrderBy(c => c.Thumbprint) + .Select(c => GetDescription(c)) + .ToArray()); internal static string GetDescription(X509Certificate2 c) => $"{c.Thumbprint[0..6]} - {c.Subject} - {c.GetEffectiveDateString()} - {c.GetExpirationDateString()} - {Instance.IsHttpsDevelopmentCertificate(c)} - {Instance.IsExportable(c)}"; diff --git a/src/Shared/CertificateGeneration/MacOSCertificateManager.cs b/src/Shared/CertificateGeneration/MacOSCertificateManager.cs index 12f6e7cc5525..1f8aea028011 100644 --- a/src/Shared/CertificateGeneration/MacOSCertificateManager.cs +++ b/src/Shared/CertificateGeneration/MacOSCertificateManager.cs @@ -56,7 +56,10 @@ protected override void TrustCertificateCore(X509Certificate2 publicCertificate) try { ExportCertificate(publicCertificate, tmpFile, includePrivateKey: false, password: null, CertificateKeyExportFormat.Pfx); - Log.MacOSTrustCommandStart($"{MacOSTrustCertificateCommandLine} {MacOSTrustCertificateCommandLineArguments}{tmpFile}"); + if (Log.IsEnabled()) + { + Log.MacOSTrustCommandStart($"{MacOSTrustCertificateCommandLine} {MacOSTrustCertificateCommandLineArguments}{tmpFile}"); + } using (var process = Process.Start(MacOSTrustCertificateCommandLine, MacOSTrustCertificateCommandLineArguments + tmpFile)) { process.WaitForExit(); @@ -238,7 +241,11 @@ private static void RemoveCertificateFromKeyChain(string keyChain, X509Certifica RedirectStandardError = true }; - Log.MacOSRemoveCertificateFromKeyChainStart(keyChain, GetDescription(certificate)); + if (Log.IsEnabled()) + { + Log.MacOSRemoveCertificateFromKeyChainStart(keyChain, GetDescription(certificate)); + } + using (var process = Process.Start(processInfo)) { var output = process.StandardOutput.ReadToEnd() + process.StandardError.ReadToEnd(); @@ -282,7 +289,11 @@ protected override X509Certificate2 SaveCertificateCore(X509Certificate2 certifi RedirectStandardError = true }; - Log.MacOSAddCertificateToKeyChainStart(MacOSUserKeyChain, GetDescription(certificate)); + if (Log.IsEnabled()) + { + Log.MacOSAddCertificateToKeyChainStart(MacOSUserKeyChain, GetDescription(certificate)); + } + using (var process = Process.Start(processInfo)) { var output = process.StandardOutput.ReadToEnd() + process.StandardError.ReadToEnd();