-
Notifications
You must be signed in to change notification settings - Fork 850
Ensure the typescript app host trusts the dev cert #15634
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
base: main
Are you sure you want to change the base?
Changes from all commits
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 |
|---|---|---|
|
|
@@ -173,6 +173,43 @@ public CertificateCleanResult CleanHttpCertificate() | |
| } | ||
| } | ||
|
|
||
| public string? ExportDevCertificatePublicPem(string outputPath) | ||
|
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. 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. |
||
| { | ||
| var availableCertificates = certificateManager.ListCertificates( | ||
| StoreName.My, StoreLocation.CurrentUser, isValid: true); | ||
|
|
||
| try | ||
| { | ||
| var now = DateTimeOffset.Now; | ||
| var certificate = availableCertificates | ||
| .Where(c => CertificateManager.IsHttpsDevelopmentCertificate(c) | ||
| && c.NotBefore <= now && now <= c.NotAfter) | ||
| .OrderByDescending(CertificateManager.GetCertificateVersion) | ||
| .ThenByDescending(c => c.NotAfter) | ||
| .FirstOrDefault(); | ||
|
|
||
| if (certificate is null) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| var directory = Path.GetDirectoryName(outputPath); | ||
| if (!string.IsNullOrEmpty(directory)) | ||
| { | ||
| Directory.CreateDirectory(directory); | ||
| } | ||
|
|
||
| var pem = certificate.ExportCertificatePem(); | ||
| File.WriteAllText(outputPath, pem); | ||
|
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. 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);
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. Up to you whether it's worth doing this, or unconditionally overwriting is better/simplier. Add a comment if so. |
||
|
|
||
| return outputPath; | ||
| } | ||
| finally | ||
| { | ||
| CertificateManager.DisposeCertificates(availableCertificates); | ||
| } | ||
| } | ||
|
|
||
| private static string[]? GetSanExtension(X509Certificate2 cert) | ||
| { | ||
| var dnsNames = new List<string>(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -345,10 +345,12 @@ public async Task<int> RunAsync(AppHostProjectContext context, CancellationToken | |
| { | ||
| // Step 1: Ensure certificates are trusted | ||
| Dictionary<string, string> certEnvVars; | ||
| string? devCertPemPath; | ||
|
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.
string? devCertPemPath = null;
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. 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. |
||
| try | ||
| { | ||
| var certResult = await _certificateService.EnsureCertificatesTrustedAsync(cancellationToken); | ||
| certEnvVars = new Dictionary<string, string>(certResult.EnvironmentVariables); | ||
| devCertPemPath = certResult.DevCertPemPath; | ||
| } | ||
| catch | ||
| { | ||
|
|
@@ -491,6 +493,10 @@ await GenerateCodeViaRpcAsync( | |
| ["ASPIRE_APPHOST_FILEPATH"] = appHostFile.FullName | ||
| }; | ||
|
|
||
| // 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 | ||
| ConfigureNodeCertificateEnvironment(environmentVariables, context.EnvironmentVariables, devCertPemPath); | ||
|
|
||
| // Pass debug flag to the guest process | ||
| if (context.Debug) | ||
| { | ||
|
|
@@ -1404,4 +1410,33 @@ private async Task<int> InstallDependenciesAsync( | |
| var id = UserSecretsPathHelper.ComputeSyntheticUserSecretsId(appHostFile.FullName); | ||
| return Task.FromResult<string?>(id); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// 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( | ||
|
Contributor
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. We need a cleaner way of doing this that comes from: |
||
| IDictionary<string, string> environmentVariables, | ||
| IDictionary<string, string> contextEnvironmentVariables, | ||
| string? devCertPemPath) | ||
| { | ||
| if (devCertPemPath is null || !LanguageId.Contains("nodejs", StringComparison.OrdinalIgnoreCase)) | ||
|
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. See other comment about improving LanguageId check. |
||
| { | ||
| return; | ||
| } | ||
|
|
||
| var existingNodeExtraCaCerts = Environment.GetEnvironmentVariable("NODE_EXTRA_CA_CERTS") | ||
| ?? (contextEnvironmentVariables.TryGetValue("NODE_EXTRA_CA_CERTS", out var ctxValue) ? ctxValue : null); | ||
|
|
||
| if (existingNodeExtraCaCerts is not null) | ||
| { | ||
| _interactionService.DisplayMessage( | ||
| KnownEmojis.Warning, | ||
| $"NODE_EXTRA_CA_CERTS is already set to '{existingNodeExtraCaCerts}'. Skipping Aspire dev certificate configuration."); | ||
| } | ||
| else | ||
| { | ||
| environmentVariables["NODE_EXTRA_CA_CERTS"] = devCertPemPath; | ||
| } | ||
| } | ||
| } | ||
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.
ExportDevCertificatePem()runs on every call toEnsureCertificatesTrustedAsync, 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.