Ensure the typescript app host trusts the dev cert#15634
Ensure the typescript app host trusts the dev cert#15634
Conversation
danegsta
requested review from
JamesNK,
davidfowl and
mitchdenny
as code owners
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 15634Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 15634" |
There was a problem hiding this comment.
Pull request overview
This PR addresses #15489 by ensuring the TypeScript (Node.js) guest AppHost process trusts the ASP.NET Core development certificate, enabling TLS connections to Aspire-managed HTTPS endpoints without disabling certificate validation in user code.
Changes:
- Export the current ASP.NET Core HTTPS dev certificate public PEM to a stable path during CLI certificate trust setup.
- Inject
NODE_EXTRA_CA_CERTSfor Node.js-based guest AppHosts so Node trusts the exported dev cert. - Add CLI unit tests covering PEM export success/failure behavior.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Aspire.Cli.Tests/Utils/CliTestHelper.cs | Updates test DI setup for CertificateService’s new constructor dependencies. |
| tests/Aspire.Cli.Tests/TestServices/TestCertificateToolRunner.cs | Extends test runner to support exporting the dev cert PEM. |
| tests/Aspire.Cli.Tests/TestServices/TestCertificateService.cs | Updates test stub result to include DevCertPemPath. |
| tests/Aspire.Cli.Tests/Certificates/CertificateServiceTests.cs | Adds tests verifying PEM export and that export failures are non-fatal. |
| src/Aspire.Hosting.CodeGeneration.TypeScript/TypeScriptLanguageSupport.cs | Formatting-only change (trailing commas). |
| src/Aspire.Cli/Projects/GuestAppHostProject.cs | Sets NODE_EXTRA_CA_CERTS for Node.js guest AppHosts based on exported PEM path. |
| src/Aspire.Cli/Certificates/NativeCertificateToolRunner.cs | Implements exporting the highest-versioned valid dev cert as PEM. |
| src/Aspire.Cli/Certificates/ICertificateToolRunner.cs | Adds contract for exporting the dev cert PEM. |
| src/Aspire.Cli/Certificates/CertificateService.cs | Exports dev cert PEM as part of ensuring certificate trust and returns the PEM path. |
| // trusts the ASP.NET Core development certificate when connecting over HTTPS | ||
| if (devCertPemPath is not null && LanguageId.Contains("nodejs", StringComparison.OrdinalIgnoreCase)) |
There was a problem hiding this comment.
This unconditionally sets NODE_EXTRA_CA_CERTS, which can override a user-provided value and potentially break environments that already rely on their own extra CA bundle. Consider only setting it when the variable is not already present, or merging the existing bundle with the exported dev cert so both are trusted.
| // trusts the ASP.NET Core development certificate when connecting over HTTPS | |
| if (devCertPemPath is not null && LanguageId.Contains("nodejs", StringComparison.OrdinalIgnoreCase)) | |
| // trusts the ASP.NET Core development certificate when connecting over HTTPS. | |
| // Do not override an existing NODE_EXTRA_CA_CERTS value provided by the user/environment. | |
| if (devCertPemPath is not null | |
| && LanguageId.Contains("nodejs", StringComparison.OrdinalIgnoreCase) | |
| && !environmentVariables.ContainsKey("NODE_EXTRA_CA_CERTS")) |
| // Set NODE_EXTRA_CA_CERTS for Node.js-based runtimes so the TypeScript app host | ||
| // trusts the ASP.NET Core development certificate when connecting over HTTPS | ||
| if (devCertPemPath is not null && LanguageId.Contains("nodejs", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| environmentVariables["NODE_EXTRA_CA_CERTS"] = devCertPemPath; | ||
| } |
There was a problem hiding this comment.
New behavior: NODE_EXTRA_CA_CERTS is injected for Node.js-based guest AppHosts, but there’s no test coverage ensuring it’s set when DevCertPemPath is returned (and that existing env var values are handled appropriately). Please add/extend a GuestAppHostProject test to validate the environment passed to the guest process includes the expected trust configuration.
JamesNK
left a comment
JamesNK
left a comment
There was a problem hiding this comment.
Looks good overall — the approach of exporting the public PEM and setting NODE_EXTRA_CA_CERTS is solid. A few suggestions inline, mostly around avoiding unnecessary work on every run.
| } | ||
|
|
||
| var pem = certificate.ExportCertificatePem(); | ||
| File.WriteAllText(outputPath, pem); |
There was a problem hiding this comment.
The PEM file is written unconditionally on every run, even if the certificate hasn't changed. This updates the file timestamp unnecessarily and does redundant I/O. Consider comparing the existing file content (or certificate thumbprint) before overwriting, e.g.:
var pem = certificate.ExportCertificatePem();
if (File.Exists(outputPath) && File.ReadAllText(outputPath) == pem)
{
return outputPath;
}
File.WriteAllText(outputPath, pem);There was a problem hiding this comment.
Up to you whether it's worth doing this, or unconditionally overwriting is better/simplier. Add a comment if so.
| { | ||
| // Step 1: Ensure certificates are trusted | ||
| Dictionary<string, string> certEnvVars; | ||
| string? devCertPemPath; |
There was a problem hiding this comment.
devCertPemPath is declared but only assigned inside the try. If anyone later changes the catch to not re-throw, it would be used uninitialized. Safer to initialize at declaration:
string? devCertPemPath = null;There was a problem hiding this comment.
Hmm, I'm surprised this compiles. I would have thought it would complain about using an uninitialized variable if it's only set in a try block.
| return new EnsureCertificatesTrustedResult | ||
| { | ||
| EnvironmentVariables = environmentVariables | ||
| EnvironmentVariables = environmentVariables, |
There was a problem hiding this comment.
ExportDevCertificatePem() runs on every call to EnsureCertificatesTrustedAsync, including for .NET app hosts where the PEM is never used. Consider making the export opt-in (e.g., a parameter or flag) or moving it to the call site that actually needs it (GuestAppHostProject), so .NET app hosts don't pay for the cert store enumeration + file write on every run.
| } | ||
| } | ||
|
|
||
| public string? ExportDevCertificatePublicPem(string outputPath) |
There was a problem hiding this comment.
There isn't any logging of what's going on here. Could you get agent to add more logging throughout this method.
There are other methods in this class that could do with more logging. Use your judgement on what's important.
Double check that nothing sensitive is logged.
|
GuestAppHostProject.cs line 496 — The if (devCertPemPath is not null && LanguageId == KnownLanguageId.TypeScript)where |
| IDictionary<string, string> contextEnvironmentVariables, | ||
| string? devCertPemPath) | ||
| { | ||
| if (devCertPemPath is null || !LanguageId.Contains("nodejs", StringComparison.OrdinalIgnoreCase)) |
There was a problem hiding this comment.
See other comment about improving LanguageId check.
|
Re-running the failed jobs in the CI workflow for this pull request because 2 jobs were identified as retry-safe transient failures in the CI run attempt.
|
|
🎬 CLI E2E Test Recordings — 52 recordings uploaded (commit View recordings
📹 Recordings uploaded automatically from CI run #23626553641 |
| /// Configures NODE_EXTRA_CA_CERTS for Node.js-based runtimes to trust the ASP.NET Core | ||
| /// development certificate. Skips configuration and warns if the variable is already set. | ||
| /// </summary> | ||
| internal void ConfigureNodeCertificateEnvironment( |
There was a problem hiding this comment.
We need a cleaner way of doing this that comes from:
Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes #15489
Checklist
<remarks />and<code />elements on your triple slash comments?aspire.devissue: