Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 13131Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 13131" |
| .WithServerAuthenticationCertificateConfiguration(async ctx => | ||
| { | ||
| builder.CreateResourceBuilder((SqlServerServerResource)ctx.Resource) | ||
| .WithContainerFiles("/var/opt/mssql/", [ |
There was a problem hiding this comment.
I'll be updating the context on the callback version of WithContainerFiles to receive the server authentication configuration (marked as experimental like the rest of the API to begin with), so this will be able to be expressed as a simple WithContainerFiles instead of needing to chain it inside WithServerAuthenticationCertificateConfiguration since it's possible certificate configuration will need to be just one part of building a config file.
There was a problem hiding this comment.
See #13151
I stuck with using ReferenceExpressions for path values even in the WithContainerFiles callback as I'm taking advantage of the ability to determine whether an particular reference was actually resolved to determine what key formats I actually need to make available for a given resource (and if no resources are actually referencing a private key, there's no reason to export any of the private key material).
There was a problem hiding this comment.
How does this look?
|
@danegsta test failure is weird. The error is semi legitimate, but it's weird that it only failed one test and not the whole lot - https://github.com/dotnet/aspire/actions/runs/19837825119/job/56839351366?pr=13131#step:22:291 . You can get the same error locally if you return an empty array from .WithContainerFiles("/tmp", async (ctx, ct) =>
{
return [];
}); |
Yeah, I need to update the handling in Aspire to skip empty results. |
|
@afscrome the empty files behavior should be fixed in main |
|
It feels like we should update the connection strings as well to enforce TLS IF SSL is enabled. |
- Updated SQL Server resource to configure a TLS cert when one is configured, and remove the TLS validation bypass from the connection string - Updated Playground app host to include sql with no TLS, and renamed resources to make their purpose clearer
There was a problem hiding this comment.
Pull request overview
Updates the SQL Server hosting integration to take advantage of the (experimental) developer certificate / HTTPS certificate plumbing so SQL Server can use a server-auth certificate and conditionally adjust connection string behavior based on whether TLS is enabled.
Changes:
- Builds SQL Server (ADO.NET) and JDBC connection strings using
EndpointReference.GetTlsValue(...)to conditionally includeTrustServerCertificate. - Enables TLS for the SQL Server endpoint via
SubscribeHttpsEndpointsUpdate(...)and generatesmssql.confwith the configured cert/key paths. - Adds shared
X509Certificate2Extensionsto support dev-cert version checks.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| src/Aspire.Hosting.SqlServer/SqlServerServerResource.cs | Refactors connection string construction and updates JDBC/connection-string docs. |
| src/Aspire.Hosting.SqlServer/SqlServerBuilderExtensions.cs | Enables TLS based on dev-cert metadata and writes mssql.conf using certificate context. |
| src/Aspire.Hosting.SqlServer/Aspire.Hosting.SqlServer.csproj | Links in shared certificate extensions used by the SQL Server integration. |
| playground/SqlServerEndToEnd/SqlServerEndToEnd.AppHost/AppHost.cs | Updates playground to exercise new certificate behavior and adds a “no TLS” SQL resource. |
Comments suppressed due to low confidence (1)
playground/SqlServerEndToEnd/SqlServerEndToEnd.AppHost/AppHost.cs:17
sqlNoTlsis created but never used. With warnings treated as errors, this will fail the build; either use it (e.g., add a database/reference) or assign it to a discard (_ = ...).
|
|
||
| if (certAnnotation == null || certAnnotation.UseDeveloperCertificate == true) | ||
| { | ||
| var devCertService = ctx.Services.GetRequiredService<IDeveloperCertificateService>(); |
There was a problem hiding this comment.
In run mode, the dev-cert version gate can be skipped when an HttpsCertificateAnnotation exists with UseDeveloperCertificate == null (default behavior). In that case, TLS gets enabled even for older dev certs, and the connection string will omit TrustServerCertificate, which is what this gate is trying to avoid. Consider computing whether a dev cert is in use via UseDeveloperCertificate.GetValueOrDefault(devCertService.UseForHttps) (and Certificate is null) rather than checking only == true.
| if (certAnnotation == null || certAnnotation.UseDeveloperCertificate == true) | |
| { | |
| var devCertService = ctx.Services.GetRequiredService<IDeveloperCertificateService>(); | |
| var devCertService = ctx.Services.GetRequiredService<IDeveloperCertificateService>(); | |
| bool useDeveloperCertificate; | |
| if (certAnnotation is null) | |
| { | |
| // No annotation: fall back to the developer certificate service setting. | |
| useDeveloperCertificate = devCertService.UseForHttps; | |
| } | |
| else if (certAnnotation.Certificate is not null) | |
| { | |
| // A custom certificate is specified; do not treat this as using the developer certificate. | |
| useDeveloperCertificate = false; | |
| } | |
| else | |
| { | |
| // No explicit certificate: compute effective dev-cert usage from the annotation value, | |
| // defaulting to the developer certificate service setting when null. | |
| useDeveloperCertificate = certAnnotation.UseDeveloperCertificate.GetValueOrDefault(devCertService.UseForHttps); | |
| } | |
| if (useDeveloperCertificate) | |
| { |
| /// <para>Format: <c>jdbc:sqlserver://{host}:{port}[];trustServerCertificate=true]</c>.</para> | ||
| /// <para>User and password credentials are not included in the JDBC connection string. | ||
| /// Use the <see cref="UserNameReference"/> and <see cref="PasswordParameter"/> connection properties to access credentials.</para> |
There was a problem hiding this comment.
The XML docs for JdbcConnectionString have malformed format text ([] / trailing ]) and now point to UserNameReference / PasswordParameter, but the connection properties exposed are named Username and Password (see IResourceWithConnectionString.GetConnectionProperties). Update the docs to reflect the actual JDBC format and reference the correct connection property names.
| /// <para>Format: <c>jdbc:sqlserver://{host}:{port}[];trustServerCertificate=true]</c>.</para> | |
| /// <para>User and password credentials are not included in the JDBC connection string. | |
| /// Use the <see cref="UserNameReference"/> and <see cref="PasswordParameter"/> connection properties to access credentials.</para> | |
| /// <para>Format: <c>jdbc:sqlserver://{Host}:{Port}[;databaseName={Database}][;trustServerCertificate=true]</c>.</para> | |
| /// <para>User and password credentials are not included in the JDBC connection string. | |
| /// Use the <c>Username</c> and <c>Password</c> connection properties to access credentials.</para> |
| var devCertService = ctx.Services.GetRequiredService<IDeveloperCertificateService>(); | ||
| var cert = devCertService.Certificates.First(); | ||
| if (cert.GetCertificateVersion() < 6) | ||
| { |
There was a problem hiding this comment.
devCertService.Certificates.First() will throw if there are no trusted dev certificates (the service can return an empty list). Guard against an empty certificate list and skip enabling TLS (or fall back to TrustServerCertificate) when no cert is available.
|
Got this working, although not entirely happy with two things
|
|
I was meaning to follow up with you on this; I merged #14919 recently which adds conditional expression support to reference expressions and should let you simplify the certificate support checks. I'm currently using this to control whether The main requirement to take advantage of this would be to expose a ReferenceExpression property from IDeveloperCertificateService that indicates whether the latest certificate supports loopback IPs as bool.TrueString/bool.FalseString values. Then you can gait a conditional value with something like I typed that up on my phone, so apologies if any of the syntax is off. |
|
Main idea of the conditional version of reference expressions is that it takes an IValueProvider as the conditional, a string that'll be compared case insensitively against the value, and two ReferenceExpression branches; the first is used if the values match, the second if they don't. |
Description
This Pr updates the SQL Server integration to use a TLS certificate. If a specific TLs cert is given, it will use that and force TLS. If no cert is configured, it'll fall back to the developer cert.
The one part that's a bit awkward is handling the
TrustServerConnectionproperty on connection strings - this attribute is required if the dev cert is older than version 6 as those versions didn't include127.0.0.1in the SAN. What I have works, but I'm not entirely happy with it.Checklist