Skip to content
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

Add better certificate configuration support to WebApplication #32124

Closed
halter73 opened this issue Apr 24, 2021 · 4 comments
Closed

Add better certificate configuration support to WebApplication #32124

halter73 opened this issue Apr 24, 2021 · 4 comments
Labels
api-approved API was approved in API review, it can be implemented area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-minimal-hosting

Comments

@halter73
Copy link
Member

halter73 commented Apr 24, 2021

Is your feature request related to a problem? Please describe.

Today if you want to configure a TLS certificate for the host in code, you have to configure Kestrel directly using WebApplicationBuilder.WebHost.ConfigureKestrel(...). Kestrel is the only server we ship that supports runtime configuration of certificates currently, but it would be nice to have a way to configure the certifcate(s) to be used by any IServer capable of using them.

It would also be nice if you could configure TLS certificates with just the WebApplication like you can for Urls. Certificates can be configured per-endpoint in Kestrel (and even per-server-name using SNI), but associating certificates with endpoints and server names could prove complicated. This is especially true with the multiple places in both code and config you can define Kestrel endpoints. However, maybe we could support this for all endpoints with a known URL.

To support this in a way that is testable and could at least theoretically work with other IServers we should add a server feature interface. IServerAddressesFeature is the only other server feature that I know of in wide use.
A clear and concise description of what the problem is.
Example: I am trying to do [...] but [...]

Describe the solution you'd like

I'm still not sure I like this, but here's one possibility:

namespace Microsoft.AspNetCore.Builder
{
    public class WebApplication
    {
+        public X509Certificate2? Certificate { get; set; }
+        public void LoadPfxCertificate(string pfxPath, string? password = null);
+        public void LoadPemCertificate(string pemPath, string keyPath, string? password = null);

           // Review: Do we need special helpers for store? Or is setting the X509Certificate2 easy enough?
           // Borrows signatures from existing UseHttps overloads.
+        public void LoadStoreCertificate(StoreName storeName, string subject);
+        public void LoadStoreCertificate(StoreName storeName, string subject, bool allowInvalid);
+        public void LoadStoreCertificate(StoreName storeName, string subject, bool allowInvalid, StoreLocation location);
namespace Microsoft.AspNetCore.Hosting.Server.Features
{
+    public interface IDefaultServerCertificateFeature
+    {
+        X509Certificate2? DefaultCertificate { get; set; }
+    }

The idea is this would basically override the default/dev cert, but certificates configured for a specific Kestrel endpoint would be preferred.

Usage Examples

using var app = WebApplication.Create(args);

// Load cert from file in content root using password from user secrets.
app.LoadPemCertificate("cert.pem", "cert.key", app.Configuration["Cert:Password"]);

app.Run("https://example.org");

Additional context

What I don't like about this proposal is that it's yet another possible source for default certificates. We will have to make sure the prioritization of these sources is clear as possible, but adding more sources can only make clarifying this harder.

@Tratcher @davidfowl

@halter73 halter73 added area-runtime enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-minimal-hosting labels Apr 24, 2021
@davidfowl davidfowl added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Apr 24, 2021
@halter73 halter73 added api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Apr 26, 2021
@BrennanConroy BrennanConroy added this to the Next sprint planning milestone Apr 28, 2021
@ghost
Copy link

ghost commented Apr 28, 2021

Thanks for contacting us.

We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@Tratcher
Copy link
Member

If IDefaultServerCertificateFeature is the source of truth, do these need to be first class methods on WebApplication? Or can they be extension methods on IApplicationBuilder? It would make them accessible to a broader audience.

IDefaultServerCertificateFeature must be supplied by the server, correct? What do these APIs do if the feature is absent (e.g. HttpSys or IIS)? Throw NotSupportedException?

Certificate should be a method rather than a property for two reasons:
A) It's disassociated from the useful methods for loading the certificate, from the property it's hard to discover the Load methods.
B) retrieving the value isn't necessary, they only need to provide one.

A more consistent approach would be:

+        public void UseCertificate(X509Certificate2 cert);
+        public void UseCertificatePfx(string pfxPath, string? password = null);
...

// Review: Do we need special helpers for store?

Can you show a one line way to retrieve the cert from the store without these?

@davidfowl
Copy link
Member

In case I can't make it to API review tomorrow:

  • I don't like Use because this type already conflates middleware configuration and routing configuration (on purpose).
  • We should avoid new top level Use/Run/Map that don't relate to middleware. If we like the term use, we might want to hang it off something else (similar to how we have WebHost and Host hanging off the builder)
  • We might want to make this work for existing ASP.NET Core applications. Should this hang off the IWebHostBuilder as well?

@pranavkm
Copy link
Contributor

namespace Microsoft.AspNetCore.Builder
{
    public class WebApplication
    {
+        public X509Certificate2? Certificate { get; set; }
    }
namespace Microsoft.AspNetCore.Hosting.Server.Features
{
+    public interface IDefaultServerCertificateFeature
+    {
+        X509Certificate2? DefaultCertificate { get; set; }
+    }

Currently approved API. @halter73 is going to speak to @javiercn about certificate loading APIs.

@pranavkm pranavkm added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels May 17, 2021
@davidfowl davidfowl removed this from the 6.0-preview5 milestone Jul 13, 2021
@rafikiassumani-msft rafikiassumani-msft added this to 6-0-rc1 All in Minimal APIs 6.0 Jul 19, 2021
Minimal APIs 6.0 automation moved this from Need review to Done Jul 19, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Aug 18, 2021
@amcasey amcasey added area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed area-runtime labels Jun 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-minimal-hosting
Projects
No open projects
Development

No branches or pull requests

8 participants
@halter73 @davidfowl @pranavkm @Tratcher @BrennanConroy @amcasey @rafikiassumani-msft and others