Ensure the typescript app host trusts the dev cert - #15634
Conversation
|
🚀 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. |
James Newton-King (JamesNK)
left a comment
James Newton-King (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.
|
GuestAppHostProject.cs line 496 — The if (devCertPemPath is not null && LanguageId == KnownLanguageId.TypeScript)where |
|
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 |
|
Any chance this gets in 13.3? |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/Aspire.Cli/Projects/GuestAppHostProject.cs:2008
- This only checks the ambient process and raw context dictionaries, but
environmentVariablesalready includes launch-profile values. Consequently, aNODE_EXTRA_CA_CERTSconfigured inlaunchSettings.jsonis not detected and is overwritten at line 2018 instead of being preserved and warned about. Check the merged dictionary as well.
var existingNodeExtraCaCerts = Environment.GetEnvironmentVariable("NODE_EXTRA_CA_CERTS")
?? (contextEnvironmentVariables.TryGetValue("NODE_EXTRA_CA_CERTS", out var ctxValue) ? ctxValue : null);
src/Aspire.Cli/Certificates/CertificateService.cs:66
- This bypasses the CLI's configured Aspire state root.
CliExecutionContext.AspireHomeDirectoryhonorsASPIRE_HOMEand install-route-specific roots (CliExecutionContext.cs:160-180), while rebuildingHomeDirectory/.aspiredoes not; with a custom writable Aspire home and a read-only user profile, export silently fails and the Node AppHost remains unable to trust the certificate. Store the PEM underAspireHomeDirectorylike other CLI state.
internal string DevCertPemPath => Path.Combine(
executionContext.HomeDirectory.FullName, ".aspire", "dev-certs", DevCertPemFileName);
src/Aspire.Cli/Certificates/NativeCertificateToolRunner.cs:195
- The new native export path has no focused coverage: the added service test replaces this method with a callback, so certificate filtering/selection and the actual PEM file contents are never exercised. Please add a
NativeCertificateToolRunnerTestscase that supplies a valid development certificate, exports to a temporary path, and verifies that the resulting PEM parses to that certificate.
public string? ExportDevCertificatePublicPem(string outputPath)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f0f2521e-b6ba-4441-9d8d-08483d863485
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f0f2521e-b6ba-4441-9d8d-08483d863485
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/Aspire.Cli/Projects/GuestAppHostProject.cs:618
- Enabling certificate bundles now makes a runtime's otherwise-valid
Languageidentifier subject to an undocumented filename restriction. Replacing/handles the built-in ID, butConfigureCertificateBundleEnvironmentAsyncrejects other common identifier characters such as.or spaces, so a custom runtime likemy.runtimewill fail AppHost startup. Derive an always-safe cache prefix (for example, by hashing or fully sanitizing the language ID) rather than throwing for unrelated metadata.
_guestRuntime.Language.Replace('/', '-'),
src/Aspire.TypeSystem/ILanguageSupport.cs:28
- This new interface member is not consumed by the runtime path:
LanguageServiceserializes onlyGetRuntimeSpec(), and the CLI readsRuntimeSpec.CertificateBundleEnvironmentVariable. A custom language implementation that follows this property's documentation but does not duplicate the value into its returnedRuntimeSpecwill silently get no certificate configuration. Please remove this redundant member or make it authoritative when producing the runtime spec.
string? CertificateBundleEnvironmentVariable => null;
This comment has been minimized.
This comment has been minimized.
PR Testing ReportPR Information
Artifact Version Verification
Changes AnalyzedThe PR changes TypeScript AppHost certificate handling in the CLI and TypeScript language support. It exports the trusted ASP.NET Core development certificate, combines it with an existing Test Scenarios ExecutedmacOS Redis TLS connectionCoverage: Happy path Status: Passed A fresh TypeScript AppHost used Redis over TLS with an existing custom CA bundle. A real Node Redis client connected through the generated Evidence:
macOS existing CA preservation, permissions, and cache reuseCoverage: Boundary Status: Passed The generated bundle retained the existing CA as its trailing bytes, contained both certificates, used mode Evidence:
Missing existing bundleCoverage: Unhappy path Status: Passed When Evidence:
Publish-mode boundaryCoverage: Boundary Status: Passed
Evidence:
Linux TypeScript AppHost TLS validationCoverage: Happy path and Linux partial-trust path Status: Passed After The bundle preserved Evidence:
Linux cache reuseCoverage: Boundary Status: Passed Two launches produced the same content-addressed bundle path and unchanged modification timestamp. Evidence:
Focused unit testsCoverage: Regression Status: Passed
Environment LimitationThe exact Redis TLS probe could not be repeated inside the Linux repo runner because DCP published the Redis port on the Docker Desktop host while the AppHost health check ran inside a sibling container. Redis remained waiting for health due to that runner network topology. The Linux test therefore used a local HTTPS server with the same development certificate, while the exact Redis TLS scenario was validated on macOS. Summary
Overall ResultPR verified. No product issues found. |
Karol Zadora-Przylecki (karolz-ms)
left a comment
Karol Zadora-Przylecki (karolz-ms)
left a comment
There was a problem hiding this comment.
Reviewed the TypeScript dev-cert trust flow end to end (language support -> RuntimeSpec -> RPC -> GuestRuntime -> run path), plus the cert selection code moved into X509Certificate2Extensions.
6 issues, none blocking on their own:
- Correctness / durability (2): unguarded temp-file cleanup that can mask the real failure or discard a successful export; combined-bundle write is not flushed to disk while the cache is validated by file name only, so a truncated bundle can be reused forever.
- Test coverage (1): nothing covers the run-path wiring or the actual reported scenario from #15489.
- Maintainability / docs (3): two drifting copies of the restricted-permission atomic-write logic, WHY comments lost when the trust check moved to shared code, and the new public extension point doesn't document its trust scope.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f0f2521e-b6ba-4441-9d8d-08483d863485
Tests selector (audit mode)The full test matrix and all jobs still run in audit mode. The tests and jobs below are what selective CI would run under enforcement. 100 / 100 test projects · 8 jobs, from 26 changed files. Selected test projects (100 / 100)
Selected jobs (8)
How these were chosen — grouped by what changed
🔧 show 89
🔧 🔧 📦 affected project 📦 affected project 🔧 🧪 🧪 🧪 🧪 🧪 🧪 🧪 🧪 🧪 🧪 🧪 🧪 🧪 🧪 Job reasons
Selection computed for commit |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/Aspire.TypeSystem/ILanguageSupport.cs:31
- Overriding this new public member does not actually opt a language into certificate injection.
LanguageService.GetRuntimeSpec()returns onlylanguageSupport.GetRuntimeSpec(), and the CLI readsRuntimeSpec.CertificateBundleEnvironmentVariable; TypeScript works only because its implementation manually copies this property into that separate object. A language provider following this property's documentation alone will see no effect. Please make one member the source of truth—either propagate this value into the returned runtime spec centrally or remove this interface member and direct providers to set theRuntimeSpecproperty.
string? CertificateBundleEnvironmentVariable => null;
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
PR Testing ReportPR Information
Artifact Version Verification
Both isolated dogfood installations matched the latest PR head before behavioral testing began. Changes AnalyzedThe PR changes TypeScript AppHost certificate handling in the CLI and TypeScript language support. It exports the trusted ASP.NET Core development certificate, combines it with an existing The latest commit also centralizes certificate-cache writes, durably flushes and atomically publishes cache files, restricts Unix permissions, handles concurrent writers, preserves trust rationale documentation, and adds end-to-end coverage for the Node environment. Change Categories
Test Scenarios ExecutedScenario 1: macOS development-certificate TLSObjective: Verify a fresh TypeScript AppHost can make a real Node HTTPS request to a server presenting the ASP.NET Core development certificate while preserving an existing CA. Coverage type: Happy path Status: Passed Node returned HTTP 200 with normal certificate validation. The generated bundle contained the development certificate first and preserved the existing custom CA bytes unchanged. Evidence:
Scenario 2: Linux development-certificate TLS and partial trustObjective: Verify the Linux partial-trust path and real Node TLS validation in the repo container runner. Coverage type: Happy path and platform boundary Status: Passed
Evidence:
Scenario 3: Secure content-addressed cache and reuseObjective: Verify restrictive permissions, atomic cache publication, temporary-file cleanup, and stable reuse across launches. Coverage type: Boundary Status: Passed on macOS and Linux On both platforms, the bundle file used mode Evidence:
Scenario 4: Missing existing CA bundleObjective: Verify a missing Coverage type: Unhappy path Status: Passed on macOS and Linux Both platforms preserved Expected outcome: Preserve the original environment value, warn clearly, and avoid synthesizing an incomplete replacement bundle. Evidence:
Scenario 5: Publish-mode boundaryObjective: Verify publish mode does not inject a development-certificate bundle or modify the existing Node CA setting. Coverage type: Boundary Status: Passed on macOS and Linux
Evidence:
Scenario 6: Focused repository testsObjective: Run the affected certificate-service, native certificate-tool, and GuestAppHost project test classes against the latest source. Coverage type: Regression Status: Passed
Environment ObservationOn macOS, detached The Linux runner used a temporary Node-enabled image because the repository runner image does not include Node. Public NuGet and npm feeds were not used; generated projects used the approved internal feeds. Summary
Overall ResultPR verified. No product issues found. |
|
Pull request created: #1493
|
|
📝 Documentation has been drafted in microsoft/aspire.dev#1493 targeting Added a new "Trusting the certificate for outbound TLS connections" subsection to the TypeScript AppHost docs page, explaining the CLI's content-addressed PEM cache under
Note This draft PR needs human review before merging. |
|
/backport to release/13.5 |
|
Started backporting to |
|
David Negstad (@danegsta) backporting to git am output$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch
Applying: Build a PEM bundle for the dev cert
Using index info to reconstruct a base tree...
M src/Aspire.Cli/Certificates/CertificateService.cs
M src/Aspire.Cli/Certificates/ICertificateToolRunner.cs
M src/Aspire.Cli/Certificates/NativeCertificateToolRunner.cs
M src/Aspire.Cli/Projects/GuestAppHostProject.cs
M src/Aspire.Hosting.CodeGeneration.TypeScript/TypeScriptLanguageSupport.cs
M tests/Aspire.Cli.Tests/Certificates/CertificateServiceTests.cs
M tests/Aspire.Cli.Tests/TestServices/TestCertificateService.cs
M tests/Aspire.Cli.Tests/TestServices/TestCertificateToolRunner.cs
M tests/Aspire.Cli.Tests/Utils/CliTestHelper.cs
Falling back to patching base and 3-way merge...
Auto-merging src/Aspire.Cli/Certificates/CertificateService.cs
CONFLICT (content): Merge conflict in src/Aspire.Cli/Certificates/CertificateService.cs
Auto-merging src/Aspire.Cli/Certificates/ICertificateToolRunner.cs
Auto-merging src/Aspire.Cli/Certificates/NativeCertificateToolRunner.cs
Auto-merging src/Aspire.Cli/Projects/GuestAppHostProject.cs
CONFLICT (content): Merge conflict in src/Aspire.Cli/Projects/GuestAppHostProject.cs
Auto-merging src/Aspire.Hosting.CodeGeneration.TypeScript/TypeScriptLanguageSupport.cs
CONFLICT (content): Merge conflict in src/Aspire.Hosting.CodeGeneration.TypeScript/TypeScriptLanguageSupport.cs
Auto-merging tests/Aspire.Cli.Tests/Certificates/CertificateServiceTests.cs
CONFLICT (content): Merge conflict in tests/Aspire.Cli.Tests/Certificates/CertificateServiceTests.cs
Auto-merging tests/Aspire.Cli.Tests/TestServices/TestCertificateService.cs
CONFLICT (content): Merge conflict in tests/Aspire.Cli.Tests/TestServices/TestCertificateService.cs
Auto-merging tests/Aspire.Cli.Tests/TestServices/TestCertificateToolRunner.cs
Auto-merging tests/Aspire.Cli.Tests/Utils/CliTestHelper.cs
CONFLICT (content): Merge conflict in tests/Aspire.Cli.Tests/Utils/CliTestHelper.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0001 Build a PEM bundle for the dev cert
Error: The process '/usr/bin/git' failed with exit code 128 |
* Build a PEM bundle for the dev cert * Use response from cert bundle directly * Combine existing Node CA certificate bundles * Refine TypeScript development certificate handling * Handle duplicate certificate environment aliases * Clarify certificate trust scope * Handle case-sensitive certificate bundle paths * Use standard Windows path comparison * Address certificate bundle review feedback --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f0f2521e-b6ba-4441-9d8d-08483d863485
Description
Ensures TypeScript AppHosts trust the ASP.NET Core development certificate when custom AppHost code opens TLS connections to Aspire-managed resources in run mode.
ASPIRE_HOME.NODE_EXTRA_CA_CERTSvalue by creating a secure, content-addressed combined bundle.Fixes #15489
Checklist
<remarks />and<code />elements on your triple slash comments?aspire.devissue: