-
Notifications
You must be signed in to change notification settings - Fork 911
Add logging if we detect the app host is running with an untrusted dev cert #14666
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
Changes from 15 commits
3c42e64
77d3818
be562bd
33f0d1e
7d95f07
b79f3cd
60fe2d2
9bf3372
1fb6076
1783692
0eb5f0c
85e6e21
30f474b
1009421
ca2f7e9
d2c166a
797e82e
4021dbc
b972a48
2f1f17c
c9c57f0
c45514a
6624ff3
a2d7ec2
80cd613
98d854b
e145d44
41e6906
8988e3f
0415f8a
6a951b3
12754bc
db607d3
1f08c4e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,19 +3,23 @@ | |
|
|
||
| using System.Buffers; | ||
| using System.Collections; | ||
| using System.Globalization; | ||
| using System.IO.Pipelines; | ||
| using System.Net.Sockets; | ||
| using System.Text; | ||
| using Aspire.Dashboard.Utils; | ||
| using Aspire.Hosting.ApplicationModel; | ||
| using Aspire.Hosting.Dcp.Process; | ||
| using Aspire.Hosting.Resources; | ||
| using Microsoft.Extensions.Configuration; | ||
| using Microsoft.Extensions.Logging; | ||
| using Microsoft.Extensions.Options; | ||
|
|
||
| namespace Aspire.Hosting.Dcp; | ||
|
|
||
| #pragma warning disable ASPIREINTERACTION001 // Type is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed. | ||
| #pragma warning disable ASPIRECERTIFICATES001 // Type is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed. | ||
| #pragma warning disable ASPIREFILESYSTEM001 // Type is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed. | ||
|
|
||
| internal sealed class DcpHost | ||
| { | ||
|
|
@@ -29,6 +33,9 @@ internal sealed class DcpHost | |
| private readonly IInteractionService _interactionService; | ||
| private readonly Locations _locations; | ||
| private readonly TimeProvider _timeProvider; | ||
| private readonly IDeveloperCertificateService _developerCertificateService; | ||
| private readonly IFileSystemService _fileSystemService; | ||
| private readonly IConfiguration _configuration; | ||
| private readonly CancellationTokenSource _shutdownCts = new(); | ||
| private Task? _logProcessorTask; | ||
|
|
||
|
|
@@ -48,7 +55,10 @@ public DcpHost( | |
| IInteractionService interactionService, | ||
| Locations locations, | ||
| DistributedApplicationModel applicationModel, | ||
| TimeProvider timeProvider) | ||
| TimeProvider timeProvider, | ||
| IDeveloperCertificateService developerCertificateService, | ||
| IFileSystemService fileSystemService, | ||
| IConfiguration configuration) | ||
| { | ||
| _loggerFactory = loggerFactory; | ||
| _logger = loggerFactory.CreateLogger<DcpHost>(); | ||
|
|
@@ -58,11 +68,15 @@ public DcpHost( | |
| _locations = locations; | ||
| _applicationModel = applicationModel; | ||
| _timeProvider = timeProvider; | ||
| _developerCertificateService = developerCertificateService; | ||
| _fileSystemService = fileSystemService; | ||
| _configuration = configuration; | ||
| } | ||
|
|
||
| public async Task StartAsync(CancellationToken cancellationToken) | ||
| { | ||
| await EnsureDcpContainerRuntimeAsync(cancellationToken).ConfigureAwait(false); | ||
| await EnsureDevelopmentCertificateTrustAsync(cancellationToken).ConfigureAwait(false); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why in the DCP host? Doesn't seem DCP related to me. What about doing it in an app host event? For example, Seb added detection for whether secrets are disable and you have persistent container in an app host event.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was purely because it's the place we're doing existing startup runtime dependency checks (the container health check also lives here). We may be able to decide on a better location, but for now this is a location of convenience. |
||
| EnsureDcpHostRunning(); | ||
| } | ||
|
|
||
|
|
@@ -122,6 +136,50 @@ internal async Task EnsureDcpContainerRuntimeAsync(CancellationToken cancellatio | |
| } | ||
| } | ||
|
|
||
| internal async Task EnsureDevelopmentCertificateTrustAsync(CancellationToken cancellationToken) | ||
| { | ||
| AspireEventSource.Instance.DevelopmentCertificateTrustCheckStart(); | ||
|
|
||
| try | ||
| { | ||
| // Check if the interaction service is available (dashboard enabled) | ||
| if (!_interactionService.IsAvailable) | ||
| { | ||
| return; | ||
| } | ||
|
danegsta marked this conversation as resolved.
Outdated
|
||
|
|
||
| // Check and warn if the developer certificate is not trusted | ||
| if (_developerCertificateService.TrustCertificate && _developerCertificateService.Certificates.Count > 0 && !await DeveloperCertificateService.IsCertificateTrustedAsync(_fileSystemService, _developerCertificateService.Certificates.First(), cancellationToken).ConfigureAwait(false)) | ||
|
danegsta marked this conversation as resolved.
Outdated
|
||
| { | ||
|
danegsta marked this conversation as resolved.
|
||
| var trustLocation = "your project folder"; | ||
| var appHostDirectory = _configuration["AppHost:Directory"]; | ||
| if (!string.IsNullOrWhiteSpace(appHostDirectory)) | ||
| { | ||
| trustLocation = $"'{appHostDirectory}'"; | ||
| } | ||
|
|
||
| var title = InteractionStrings.DeveloperCertificateNotFullyTrustedTitle; | ||
| var message = string.Format(CultureInfo.CurrentCulture, InteractionStrings.DeveloperCertificateNotFullyTrustedMessage, trustLocation); | ||
|
|
||
| _logger.LogWarning("{Message}", message); | ||
|
danegsta marked this conversation as resolved.
Outdated
|
||
|
|
||
|
danegsta marked this conversation as resolved.
Outdated
|
||
| // Send notification to the dashboard | ||
| _ = _interactionService.PromptNotificationAsync( | ||
| title: title, | ||
| message: message, | ||
| options: new NotificationInteractionOptions | ||
| { | ||
| Intent = MessageIntent.Error, | ||
| }, | ||
| cancellationToken: cancellationToken); | ||
| } | ||
| } | ||
| finally | ||
| { | ||
| AspireEventSource.Instance.DevelopmentCertificateTrustCheckStop(); | ||
| } | ||
| } | ||
|
|
||
| public async Task StopAsync() | ||
| { | ||
| _shutdownCts.Cancel(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,10 @@ | ||
| #pragma warning disable ASPIRECERTIFICATES001 | ||
| #pragma warning disable ASPIREFILESYSTEM001 | ||
|
|
||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using Aspire.Hosting.Dcp.Process; | ||
| using Aspire.Hosting.Utils; | ||
| using Microsoft.Extensions.Configuration; | ||
| using Microsoft.Extensions.Logging; | ||
|
|
@@ -19,6 +21,10 @@ internal class DeveloperCertificateService : IDeveloperCertificateService | |
|
|
||
| public DeveloperCertificateService(ILogger<DeveloperCertificateService> logger, IConfiguration configuration, DistributedApplicationOptions options) | ||
| { | ||
| TrustCertificate = configuration.GetBool(KnownConfigNames.DeveloperCertificateDefaultTrust) ?? | ||
| options.TrustDeveloperCertificate ?? | ||
| true; | ||
|
|
||
| _certificates = new Lazy<ImmutableList<X509Certificate2>>(() => | ||
| { | ||
| try | ||
|
|
@@ -33,26 +39,32 @@ public DeveloperCertificateService(ILogger<DeveloperCertificateService> logger, | |
| // so we want to ensure the certificate that will be used by ASP.NET Core is the first one in the bundle. | ||
| // Match the ordering logic ASP.NET Core uses, including DateTimeOffset.Now for current time: https://github.com/dotnet/aspnetcore/blob/0aefdae365ff9b73b52961acafd227309524ce3c/src/Shared/CertificateGeneration/CertificateManager.cs#L122 | ||
| var now = DateTimeOffset.Now; | ||
|
|
||
| // Get all valid ASP.NET Core development certificates | ||
| var validCerts = store.Certificates | ||
| .Where(c => c.IsAspNetCoreDevelopmentCertificate()) | ||
| .Where(c => c.NotBefore <= now && now <= c.NotAfter) | ||
| .ToList(); | ||
|
|
||
| // If any certificate has a Subject Key Identifier extension, exclude certificates without it | ||
| if (validCerts.Any(c => c.HasSubjectKeyIdentifier())) | ||
| { | ||
| validCerts = validCerts.Where(c => c.HasSubjectKeyIdentifier()).ToList(); | ||
| } | ||
|
|
||
| // Take the highest version valid certificate for each unique SKI | ||
| devCerts.AddRange( | ||
| validCerts | ||
| .GroupBy(c => c.Extensions.OfType<X509SubjectKeyIdentifierExtension>().FirstOrDefault()?.SubjectKeyIdentifier) | ||
| .SelectMany(g => g.OrderByDescending(c => c.GetCertificateVersion()).ThenByDescending(c => c.NotAfter).Take(1)) | ||
| .OrderByDescending(c => c.GetCertificateVersion()).ThenByDescending(c => c.NotAfter)); | ||
|
|
||
| // Release the unused certificates | ||
| foreach (var unusedCert in validCerts.Except(devCerts)) | ||
| { | ||
| unusedCert.Dispose(); | ||
| } | ||
|
|
||
| if (devCerts.Count == 0) | ||
| { | ||
| logger.LogInformation("No ASP.NET Core developer certificates found in the CurrentUser/My certificate store."); | ||
|
|
@@ -82,15 +94,10 @@ public DeveloperCertificateService(ILogger<DeveloperCertificateService> logger, | |
| return supportsTlsTermination; | ||
| }); | ||
|
|
||
| // Environment variable config > DistributedApplicationOptions > default true | ||
| TrustCertificate = configuration.GetBool(KnownConfigNames.DeveloperCertificateDefaultTrust) ?? | ||
| options.TrustDeveloperCertificate ?? | ||
| true; | ||
|
|
||
| // By default, only use for server authentication if trust is also enabled (and a developer certificate with a private key is available) | ||
| UseForHttps = (configuration.GetBool(KnownConfigNames.DeveloperCertificateDefaultHttpsTermination) ?? | ||
| options.DeveloperCertificateDefaultHttpsTerminationEnabled ?? | ||
| true ) && TrustCertificate && _supportsTlsTermination.Value; | ||
| true) && TrustCertificate && _supportsTlsTermination.Value; | ||
| } | ||
|
|
||
| /// <inheritdoc /> | ||
|
|
@@ -104,4 +111,71 @@ public DeveloperCertificateService(ILogger<DeveloperCertificateService> logger, | |
|
|
||
| /// <inheritdoc /> | ||
| public bool UseForHttps { get; } | ||
|
|
||
| internal static async Task<bool> IsCertificateTrustedAsync(IFileSystemService fileSystemService, X509Certificate2 certificate, CancellationToken cancellationToken = default) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Someone else will have to review this. I'm not an expert in this area.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was reviewed in the previous version of the PR that stalled out due to a test issue that's now fixed. I re-opened because the old one was targeting main instead of release/13.2. |
||
| { | ||
| ArgumentNullException.ThrowIfNull(certificate); | ||
|
|
||
| if (OperatingSystem.IsMacOS()) | ||
| { | ||
| // On MacOS we have to verify against the Keychain | ||
| return await IsCertificateTrustedInMacOsKeychainAsync(fileSystemService, certificate, cancellationToken).ConfigureAwait(false); | ||
| } | ||
|
|
||
| try | ||
| { | ||
| // On Linux and Windows, we need to check if the certificate is in the Root store | ||
| using var store = new X509Store(StoreName.Root, StoreLocation.CurrentUser); | ||
| store.Open(OpenFlags.ReadOnly); | ||
|
|
||
| var matches = store.Certificates.Find(X509FindType.FindByThumbprint, certificate.Thumbprint, validOnly: false); | ||
|
danegsta marked this conversation as resolved.
Outdated
|
||
| try | ||
| { | ||
| return matches.Count > 0; | ||
| } | ||
| finally | ||
| { | ||
| foreach (var cert in matches) | ||
| { | ||
| cert.Dispose(); | ||
| } | ||
| } | ||
| } | ||
| catch | ||
| { | ||
| // Ignore errors and assume not trusted | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| // Use the same approach as `dotnet dev-certs` to check if the certificate is trusted in the macOS keychain | ||
| // See: https://github.com/dotnet/aspnetcore/blob/2a88012113497bac5056548f16d810738b069198/src/Shared/CertificateGeneration/MacOSCertificateManager.cs#L36-L37 | ||
| private static async Task<bool> IsCertificateTrustedInMacOsKeychainAsync(IFileSystemService fileSystemService, X509Certificate2 certificate, CancellationToken cancellationToken) | ||
| { | ||
| try | ||
| { | ||
| using var tempDirectory = fileSystemService.TempDirectory.CreateTempSubdirectory("aspire-devcert-check"); | ||
| var certPath = Path.Combine(tempDirectory.Path, $"aspire-devcert-{certificate.Thumbprint}.cer"); | ||
|
|
||
| File.WriteAllBytes(certPath, certificate.Export(X509ContentType.Cert)); | ||
|
|
||
| var processSpec = new ProcessSpec("security") | ||
| { | ||
| Arguments = $"verify-cert -p basic -p ssl -c {certPath}", | ||
|
danegsta marked this conversation as resolved.
Outdated
|
||
| ThrowOnNonZeroReturnCode = false | ||
| }; | ||
|
|
||
| var (task, processDisposable) = ProcessUtil.Run(processSpec); | ||
| await using (processDisposable.ConfigureAwait(false)) | ||
| { | ||
| var result = await task.WaitAsync(cancellationToken).ConfigureAwait(false); | ||
| return result.ExitCode == 0; | ||
| } | ||
| } | ||
| catch | ||
| { | ||
| // Ignore errors and assume not trusted | ||
| return false; | ||
| } | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.