-
Notifications
You must be signed in to change notification settings - Fork 797
Improve Aspire CLI dev cert check/trust process for partial trust on Linux #13917
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
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 13917Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 13917" |
|
Potential gap with OpenSUSE support The current implementation only handles certificate directories (
The Suggestions:
This may be acceptable for an initial implementation with a follow-up issue to expand distro coverage. |
|
E2E Testing Approach Here's a prompt that could be used to create a CLI E2E test scenario to validate this feature: Dev Cert Partial Trust on LinuxOverviewValidate that when running on Linux with a partially trusted dev certificate, the Aspire CLI automatically configures Prerequisites
Test Steps
Success Criteria
Failure Criteria
This test is Linux-specific and requires a controlled environment where |
|
Fix aspire doctor as well. |
|
@copilot make sure the |
Added |
Already addressed in 55756e0. The
The check respects the |
…dling on Linux Co-authored-by: DamianEdwards <249088+DamianEdwards@users.noreply.github.com>
…d minimum Co-authored-by: DamianEdwards <249088+DamianEdwards@users.noreply.github.com>
…v-certs trust path Co-authored-by: DamianEdwards <249088+DamianEdwards@users.noreply.github.com>
…R check to aspire doctor Co-authored-by: DamianEdwards <249088+DamianEdwards@users.noreply.github.com>
- Add CertificateTrustLevel enum (None, Partial, Full) - Replace IsCertificateTrusted() with GetCertificateTrustLevel() - Show 'partially trusted' warning when SSL_CERT_DIR not configured - Integrate SSL_CERT_DIR check into main trust level logic - Provide actionable fix instructions for users
a3773bb to
9ec2257
Compare
- Add DoctorCommandTests.cs with two Hex1b-based E2E tests: - DoctorCommand_WithoutSslCertDir_ShowsPartiallyTrusted - DoctorCommand_WithSslCertDir_ShowsTrusted - Add ClearSslCertDir() and ConfigureSslCertDir() helper methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR improves the Aspire CLI's development certificate trust checking on Linux by utilizing the new machine-readable output from dotnet dev-certs (available in .NET 10 SDK) and automatically configuring SSL_CERT_DIR when certificates are only partially trusted.
Changes:
- Introduced machine-readable certificate trust checking via
CheckHttpCertificateMachineReadableAsyncthat parses JSON output containing trust level information (Full/Partial/None) - Automatically configures
SSL_CERT_DIRenvironment variable on Linux when certificates are partially trusted, including OpenSSL integration to dynamically query system certificate directories - Updated certificate service to return environment variables that get passed through to AppHost processes, eliminating the need for manual
SSL_CERT_DIRconfiguration - Enhanced
aspire doctorcommand to detect and warn about partial trust scenarios on Linux - Removed legacy
CheckHttpCertificateAsyncmethod since .NET 10 SDK is now the minimum requirement
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Aspire.Cli/Certificates/CertificateService.cs | Implements new machine-readable trust checking logic, automatic SSL_CERT_DIR configuration, and OpenSSL integration |
| src/Aspire.Cli/Certificates/DevCertInfo.cs | Defines data models for certificate trust information from machine-readable output |
| src/Aspire.Cli/DotNet/DotNetCliRunner.cs | Adds CheckHttpCertificateMachineReadableAsync method that parses JSON output from dev-certs |
| src/Aspire.Cli/Projects/DotNetAppHostProject.cs | Applies certificate environment variables when launching AppHost |
| src/Aspire.Cli/Projects/GuestAppHostProject.cs | Applies certificate environment variables when launching guest AppHost |
| src/Aspire.Cli/Utils/EnvironmentChecker/DevCertsCheck.cs | Adds partial trust detection and SSL_CERT_DIR configuration warnings |
| src/Aspire.Cli/Commands/InitCommand.cs | Updates to use new certificate service signature |
| src/Aspire.Cli/Templating/DotNetTemplateFactory.cs | Updates to use new certificate service signature |
| src/Aspire.Cli/JsonSourceGenerationContext.cs | Adds JSON serialization support for certificate info list |
| tests/Aspire.Cli.Tests/Certificates/CertificateServiceTests.cs | Comprehensive unit tests for all trust scenarios (full, partial, none) |
| tests/Aspire.Cli.Tests/TestServices/TestDotNetCliRunner.cs | Updates test service to support new machine-readable check |
| tests/Aspire.Cli.Tests/Commands/RunCommandTests.cs | Removes obsolete certificate check callback setup |
| tests/Aspire.Cli.Tests/Commands/NewCommandTests.cs | Updates mock certificate service |
| tests/Aspire.Cli.Tests/Templating/DotNetTemplateFactoryTests.cs | Updates test implementations for new signatures |
| tests/Aspire.Cli.EndToEnd.Tests/DoctorCommandTests.cs | New E2E tests validating partial trust detection in aspire doctor |
| tests/Aspire.Cli.EndToEnd.Tests/Helpers/CliE2ETestHelpers.cs | Helper methods for SSL_CERT_DIR manipulation in E2E tests |
| .github/workflows/cli-e2e-recording-comment.yml | Adds DoctorCommandTests to recording artifact collection |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// <summary> | ||
| /// Represents the trust level of a development certificate. | ||
| /// </summary> | ||
| internal static class DevCertTrustLevel | ||
| { | ||
| /// <summary> | ||
| /// The certificate is fully trusted by all trust stores. | ||
| /// </summary> | ||
| public const string Full = "Full"; | ||
|
|
||
| /// <summary> | ||
| /// The certificate is partially trusted (e.g., SSL_CERT_DIR not configured on Linux). | ||
| /// </summary> | ||
| public const string Partial = "Partial"; | ||
|
|
||
| /// <summary> | ||
| /// The certificate is not trusted. | ||
| /// </summary> | ||
| public const string None = "None"; | ||
| } |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation for these internal constants is too verbose. According to the coding guidelines, internal APIs should have brief summary tags only. These constants should have minimal documentation like "Full trust level constant" rather than full sentences.
| /// <summary> | ||
| /// Gets the dev-certs trust path, respecting the DOTNET_DEV_CERTS_OPENSSL_CERTIFICATE_DIRECTORY override. | ||
| /// </summary> | ||
| private static string GetDevCertsTrustPath() | ||
| { | ||
| var overridePath = Environment.GetEnvironmentVariable(DevCertsOpenSslCertDirEnvVar); | ||
| return !string.IsNullOrEmpty(overridePath) ? overridePath : s_defaultDevCertsTrustPath; | ||
| } |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation for this internal method is too verbose. According to the coding guidelines, internal APIs should have brief summary tags only. The detailed explanation about respecting environment variable overrides should be simplified or removed.
| /// <summary> | ||
| /// Attempts to get the OpenSSL certificates directory by running 'openssl version -d'. | ||
| /// This is the same approach used by ASP.NET Core's certificate manager. | ||
| /// </summary> | ||
| /// <param name="certsDir">The path to the OpenSSL certificates directory if found.</param> | ||
| /// <returns>True if the OpenSSL certs directory was found, false otherwise.</returns> | ||
| private static bool TryGetOpenSslCertsDirectory([NotNullWhen(true)] out string? certsDir) |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation for this internal method is too verbose. According to the coding guidelines, internal APIs should have brief summary tags only. The detailed explanation about ASP.NET Core's approach should be simplified to just describe what the method does.
| /// <summary> | ||
| /// Gets the dev-certs trust path, respecting the DOTNET_DEV_CERTS_OPENSSL_CERTIFICATE_DIRECTORY override. | ||
| /// </summary> | ||
| private static string GetDevCertsTrustPath() | ||
| { | ||
| var overridePath = Environment.GetEnvironmentVariable(DevCertsOpenSslCertDirEnvVar); | ||
| return !string.IsNullOrEmpty(overridePath) | ||
| ? overridePath | ||
| : Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), ".aspnet", "dev-certs", "trust"); | ||
| } |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation for this internal method is too verbose. According to the coding guidelines, internal APIs should have brief summary tags only. The detailed explanation about respecting the environment variable override should be simplified.
| /// <summary> | ||
| /// Checks if SSL_CERT_DIR is configured to include the dev-certs trust path. | ||
| /// </summary> | ||
| private static bool IsSslCertDirConfigured() |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation for this internal method is too verbose. According to the coding guidelines, internal APIs should have brief summary tags only. The detailed explanation about what SSL_CERT_DIR does should be simplified.
| /// <summary> | ||
| /// Represents information about a development certificate from `dotnet dev-certs https --check-trust-machine-readable`. | ||
| /// </summary> | ||
| internal sealed class DevCertInfo |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation for these internal classes and members is too verbose. According to the coding guidelines, internal APIs should have brief summary tags only, without verbose remarks or examples. These are internal types used only by maintainers, not public consumption.
| /// <summary> | ||
| /// The result of checking certificate trust status using machine-readable output. | ||
| /// </summary> | ||
| internal sealed class CertificateTrustResult |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation for this internal class is too verbose. According to the coding guidelines, internal APIs should have brief summary tags only. The detailed explanation of what the result contains should be simplified or removed since this is an internal type.
| /// <summary> | ||
| /// The result of ensuring certificates are trusted. | ||
| /// </summary> | ||
| internal sealed class EnsureCertificatesTrustedResult | ||
| { | ||
| /// <summary> | ||
| /// Gets the environment variables that should be set for the AppHost process | ||
| /// to ensure certificates are properly trusted. | ||
| /// </summary> | ||
| public required IDictionary<string, string> EnvironmentVariables { get; init; } | ||
| } |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation for this internal class is too verbose. According to the coding guidelines, internal APIs should have brief summary tags only. The detailed explanation of environment variables and their purpose should be simplified since this is an internal type.
| /// <summary> | ||
| /// Represents the trust level of a certificate. | ||
| /// </summary> | ||
| internal enum CertificateTrustLevel | ||
| { | ||
| /// <summary>Certificate is not in any trusted store.</summary> | ||
| None, | ||
| /// <summary>Certificate is in a trusted store but SSL_CERT_DIR is not configured (Linux only).</summary> | ||
| Partial, | ||
| /// <summary>Certificate is fully trusted.</summary> | ||
| Full | ||
| } |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation for this internal enum is too verbose. According to the coding guidelines, internal APIs should have brief summary tags only. The detailed explanations for each enum value should be simplified to concise descriptions.
| /// <summary> | ||
| /// Checks if a certificate is trusted by the system. | ||
| /// Gets the trust level of a certificate. | ||
| /// </summary> | ||
| private bool IsCertificateTrusted(X509Certificate2 certificate) | ||
| private CertificateTrustLevel GetCertificateTrustLevel(X509Certificate2 certificate) |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation for this internal method is too verbose. According to the coding guidelines, internal APIs should have brief summary tags only. The detailed explanation about the trust level logic should be simplified to just describe what the method does.
|
@copilot rebase the branch to resolve the conflicts |
I'm unable to rebase the branch due to environment limitations - I cannot use |
🎬 CLI E2E Test RecordingsThe following terminal recordings are available for commit
📹 Recordings uploaded automatically from CI run #21228843148 |
Description
On Linux,
dotnet dev-certs https --trust --checkalways fails whenSSL_CERT_DIRis not set, but the trust operation succeeds regardless. This causesaspire runto attempt certificate trust on every invocation.This PR uses
dotnet dev-certs https --check-trust-machine-readable(.NET 10 SDK) to get detailed trust status and automatically configuresSSL_CERT_DIRwhen certificates are "partially" trusted. Since the CLI requires .NET 10 SDK minimum, the machine-readable check is always available.Key changes:
CheckHttpCertificateMachineReadableAsyncparses JSON output withTrustLevel(Full/Partial/None)SSL_CERT_DIRto include system cert dirs + the dev-certs trust directoryEnsureCertificatesTrustedAsyncnow returns env vars to apply when launching AppHostCheckHttpCertificateAsyncmethod since .NET 10 SDK is the minimum requirementDOTNET_DEV_CERTS_OPENSSL_CERTIFICATE_DIRECTORYenvironment variable overrideopenssl version -dto dynamically query the OpenSSL certificates directory (same approach as ASP.NET Core'sUnixCertificateManager)SSL_CERT_DIRconfiguration check toaspire doctorcommand to warn users about partial trust issues on LinuxChecklist
<remarks />and<code />elements on your triple slash comments?doc-ideatemplatebreaking-changetemplatediagnostictemplateOriginal prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.