From 4d6a9b241c98b3c65ff96902bb2f0b31a1e43f2c Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Wed, 16 Oct 2024 12:23:23 -0700 Subject: [PATCH 1/2] Improve dev-certs export error message During a recent security review of the dev-certs tool, we observed that on export it would create a directory that was potentially world-readable (e.g. based on permissions inherited from the parent directory). We decided it would be more appropriate to let users make the decision of who should have access to the directory. Unfortunately, this removal of functionality broke some app authors' workflows. When dev-certs is run directly, the `--verbose` output makes it clear what went wrong and what needs to happen, but the non-verbose output that appears when another tool does the export is less helpful. This change introduces a new top-level error state for an export failure caused by a non-existent target directory to make it clearer how to fix broken workflows. The behavior changed in #57108, which included a backport of #56985, and shipped in 8.0.10. For #58330 --- src/Shared/CertificateGeneration/CertificateManager.cs | 1 + src/Shared/CertificateGeneration/EnsureCertificateResult.cs | 1 + .../FirstRunCertGenerator/test/CertificateManagerTests.cs | 2 +- src/Tools/dotnet-dev-certs/src/Program.cs | 4 ++++ 4 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/Shared/CertificateGeneration/CertificateManager.cs b/src/Shared/CertificateGeneration/CertificateManager.cs index 96e5630c43f1..491eb5adb974 100644 --- a/src/Shared/CertificateGeneration/CertificateManager.cs +++ b/src/Shared/CertificateGeneration/CertificateManager.cs @@ -328,6 +328,7 @@ public EnsureCertificateResult EnsureAspNetCoreHttpsDevelopmentCertificate( var exportDir = Path.GetDirectoryName(path); if (!string.IsNullOrEmpty(exportDir) && !Directory.Exists(exportDir)) { + result = EnsureCertificateResult.ErrorExportingTheCertificateToNonExistentDirectory; throw new InvalidOperationException($"The directory '{exportDir}' does not exist. Choose permissions carefully when creating it."); } diff --git a/src/Shared/CertificateGeneration/EnsureCertificateResult.cs b/src/Shared/CertificateGeneration/EnsureCertificateResult.cs index cb6b7e145428..5c28eaca3063 100644 --- a/src/Shared/CertificateGeneration/EnsureCertificateResult.cs +++ b/src/Shared/CertificateGeneration/EnsureCertificateResult.cs @@ -10,6 +10,7 @@ internal enum EnsureCertificateResult ErrorCreatingTheCertificate, ErrorSavingTheCertificateIntoTheCurrentUserPersonalStore, ErrorExportingTheCertificate, + ErrorExportingTheCertificateToNonExistentDirectory, FailedToTrustTheCertificate, PartiallyFailedToTrustTheCertificate, UserCancelledTrustStep, diff --git a/src/Tools/FirstRunCertGenerator/test/CertificateManagerTests.cs b/src/Tools/FirstRunCertGenerator/test/CertificateManagerTests.cs index 09a32825e50e..bcc2d6ef05b5 100644 --- a/src/Tools/FirstRunCertGenerator/test/CertificateManagerTests.cs +++ b/src/Tools/FirstRunCertGenerator/test/CertificateManagerTests.cs @@ -373,7 +373,7 @@ public void EnsureCreateHttpsCertificate_CannotExportToNonExistentDirectory() .EnsureAspNetCoreHttpsDevelopmentCertificate(now, now.AddYears(1), Path.Combine("NoSuchDirectory", CertificateName)); // Assert - Assert.Equal(EnsureCertificateResult.ErrorExportingTheCertificate, result); + Assert.Equal(EnsureCertificateResult.ErrorExportingTheCertificateToNonExistentDirectory, result); } [Fact] diff --git a/src/Tools/dotnet-dev-certs/src/Program.cs b/src/Tools/dotnet-dev-certs/src/Program.cs index 9f5407d9d3e4..bb1accc3ab35 100644 --- a/src/Tools/dotnet-dev-certs/src/Program.cs +++ b/src/Tools/dotnet-dev-certs/src/Program.cs @@ -425,6 +425,10 @@ private static int EnsureHttpsCertificate(CommandOption exportPath, CommandOptio case EnsureCertificateResult.ErrorExportingTheCertificate: reporter.Warn("There was an error exporting the HTTPS developer certificate to a file."); return ErrorExportingTheCertificate; + case EnsureCertificateResult.ErrorExportingTheCertificateToNonExistentDirectory: + // A distinct warning is useful, but a distinct error code is probably not. + reporter.Warn("There was an error exporting the HTTPS developer certificate to a file. Please create the target directory before exporting."); + return ErrorExportingTheCertificate; case EnsureCertificateResult.PartiallyFailedToTrustTheCertificate: // A distinct warning is useful, but a distinct error code is probably not. reporter.Warn("There was an error trusting the HTTPS developer certificate. It will be trusted by some clients but not by others."); From 0ef6084fdd8adb1844da3ca72f5b202b09f84b44 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Wed, 16 Oct 2024 13:12:02 -0700 Subject: [PATCH 2/2] Improve error text --- src/Tools/dotnet-dev-certs/src/Program.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Tools/dotnet-dev-certs/src/Program.cs b/src/Tools/dotnet-dev-certs/src/Program.cs index bb1accc3ab35..9b195dbcd9fa 100644 --- a/src/Tools/dotnet-dev-certs/src/Program.cs +++ b/src/Tools/dotnet-dev-certs/src/Program.cs @@ -427,7 +427,7 @@ private static int EnsureHttpsCertificate(CommandOption exportPath, CommandOptio return ErrorExportingTheCertificate; case EnsureCertificateResult.ErrorExportingTheCertificateToNonExistentDirectory: // A distinct warning is useful, but a distinct error code is probably not. - reporter.Warn("There was an error exporting the HTTPS developer certificate to a file. Please create the target directory before exporting."); + reporter.Warn("There was an error exporting the HTTPS developer certificate to a file. Please create the target directory before exporting. Choose permissions carefully when creating it."); return ErrorExportingTheCertificate; case EnsureCertificateResult.PartiallyFailedToTrustTheCertificate: // A distinct warning is useful, but a distinct error code is probably not.