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

Support specifying a full certificate chain in HttpsConnectionAdapterOptions #21513

Closed
davidfowl opened this issue May 5, 2020 · 36 comments
Closed
Assignees
Labels
affected-very-few This issue impacts very few customers api-approved API was approved in API review, it can be implemented area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions Done This issue has been fixed enhancement This issue represents an ask for new feature or an enhancement to an existing one ✔️ Resolution: Fixed The bug or enhancement requested in this issue has been checked-in!
Milestone

Comments

@davidfowl
Copy link
Member

davidfowl commented May 5, 2020

public class HttpsConnectionAdapterOptions
{
    public X509Certificate2 ServerCertificate { get; set; }
+   public X509Certificate2Collection ServerCertificateChain { get; set; }
}

Configuration:

For configuration, we'll support the full cert chain in Path:

// "CertificateName": {
     //     "Path": "testCert.pem/pfx",
     //     "KeyPath": "key.pem",

This depends on dotnet/runtime#35844

@Pilchie
Copy link
Member

Pilchie commented Sep 2, 2020

Moving this to RC2, since the PR isn't ready yet, and we're out of time for RC1

@ghost
Copy link

ghost commented Sep 25, 2020

Thanks for contacting us.
We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We will evaluate the request when we are planning the work for the next milestone. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@jkotalik jkotalik added affected-very-few This issue impacts very few customers enhancement This issue represents an ask for new feature or an enhancement to an existing one severity-nice-to-have This label is used by an internal tool labels Nov 13, 2020 — with ASP.NET Core Issue Ranking
@davidfowl
Copy link
Member Author

Closed PR #24935 (lack of tests)

@davidfowl davidfowl added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Mar 23, 2021
@mkArtakMSFT
Copy link
Member

@davidfowl it'll be handier to review the PR. So not reviewing this yet.

@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 Apr 7, 2021
@junsaw
Copy link

junsaw commented Apr 16, 2021

Is this issue closed ?

@davidfowl
Copy link
Member Author

davidfowl commented Apr 17, 2021

No the issue isn't closed. If you want to take a stab at this, you can revive this PR #24935 and write a test. We need to generate a valid certificate chain (@blowdart do you still have one) in order to write a test.

@blowdart
Copy link
Contributor

Hah no. I emailed you the instructions at one point. Not sure where they are now to be honest.

@davidfowl
Copy link
Member Author

Did you do it purely with an openssl configuration?

@jstachowiak
Copy link

Love it! This will simplify a lot our use case where we generate dynamic certificates at schedule time with Vault and have a few intermediates in the bundle that need to be sent over as well.

@jstachowiak
Copy link

@davidfowl, if having a test CA chain is a blocker then you can easily generate one with the help of HashiCorp Vault API. Just follow the steps from here and assume that:

  • Root CA is setup at pki/
  • Intermediate CA is setup at pki_int/

You can have as many intermediates as you want.

@MarkCiliaVincenti
Copy link

@Tratcher, thanks so much! Fixed and released the NuGet package TlsCertificateLoader 1.0.4 with working refresh :)

@halter73
Copy link
Member

options.ListenAnyIp(433, o =>
{
     o.SetTlsHandshakeCallbackOptions(fullChainPath, privateKeyPath);
     o.SetHttpsConnectionAdapterOptions(fullChainPath, privateKeyPath);
     o.Protocols = HttpProtocols.Http1AndHttp2AndHttp3;
});

Only the first call to UseHttps() should have any impact. I don't think o.SetHttpsConnectionAdapterOptions(fullChainPath, privateKeyPath); does anything in the example.

I'd consider removing SetHttpsConnectionAdapterOptions. The TlsHandshakeCallbackOptions UseHttps() overload should be preferred wherever possible.

@MarkCiliaVincenti
Copy link

MarkCiliaVincenti commented Nov 11, 2021

options.ListenAnyIp(433, o =>
{
     o.SetTlsHandshakeCallbackOptions(fullChainPath, privateKeyPath);
     o.SetHttpsConnectionAdapterOptions(fullChainPath, privateKeyPath);
     o.Protocols = HttpProtocols.Http1AndHttp2AndHttp3;
});

Only the first call to UseHttps() should have any impact. I don't think o.SetHttpsConnectionAdapterOptions(fullChainPath, privateKeyPath); does anything in the example.

I'd consider removing SetHttpsConnectionAdapterOptions. The TlsHandshakeCallbackOptions UseHttps() overload should be preferred wherever possible.

With all due respect, this is incorrect. It didn't work on 6.0rc2 and I thought maybe you're right and they fixed it for 6.0 so let me revisit it on my 6.0 app and tried to disable SetHttpsConnectionAdapterOptions. The site wouldn't load and was giving a ERR_QUIC_PROTOCOL_ERROR. You absolutely need both lines if you want to use HTTP/3, at least on my environment (Ubuntu). Haven't tested on Windows, but if you want portable code anyway....

The library has been rewritten
https://github.com/MarkCiliaVincenti/TlsCertificateLoader
https://www.nuget.org/packages/TlsCertificateLoader

As far as I can tell, it's the only one that allows you to load a full chain HTTP/3 site on Kestrel.

The only thing you can't do, and I'm not convinced is possible by Kestrel, is to load multiple certificates and serve the appropriate one depending on the hostname. For example serving a certificate for mydomain.tld and a different one for www.mydomain.tld or myotherdomain.tld. HttpsConnectionAdapterOptions allows you that flexibility but TlsHandshakeCallbackOptions doesn't seem like it does, and you absolutely need the latter because the former doesn't allow the full chain anyway.

@Tratcher
Copy link
Member

Yeah, HTTP/3 is a special case because some of the callbacks don't work yet.

The only thing you can't do, and I'm not convinced is possible by Kestrel, is to load multiple certificates and serve the appropriate one depending on the hostname. For example serving a certificate for mydomain.tld and a different one for www.mydomain.tld or myotherdomain.tld. HttpsConnectionAdapterOptions allows you that flexibility but TlsHandshakeCallbackOptions doesn't seem like it does, and you absolutely need the latter because the former doesn't allow the full chain anyway.

This is possible. The TlsHandshakeCallbackOptions.OnConnection callback gives you a TlsHandshakeCallbackContext. That has the SslClientHelloInfo on it with the ServerName.

@MarkCiliaVincenti
Copy link

MarkCiliaVincenti commented Nov 11, 2021 via email

@Tratcher
Copy link
Member

That dictionary lookup goes here:
https://github.com/MarkCiliaVincenti/TlsCertificateLoader/blob/6f1b4ff27c320fa14ec990b2408b367d81f2553d/TlsCertificateLoader/Extensions/ListenOptionsExtensions.cs#L25

@MarkCiliaVincenti
Copy link

I've made some big changes to TlsCertificateLoader. I've now added middleware for Certbot certificates.

https://github.com/MarkCiliaVincenti/TlsCertificateLoader
https://www.nuget.org/packages/TlsCertificateLoader

@davidfowl
Copy link
Member Author

davidfowl commented Nov 27, 2021

Why is it 2 method calls?

@MarkCiliaVincenti
Copy link

MarkCiliaVincenti commented Nov 27, 2021 via email

@davidfowl
Copy link
Member Author

These 3 calls. It's not clear to me which calls are required. I see AddTlsCertificateLoader, UseTlsCertificateLoader, and the middleware (UseTlsCertificateLoader). The middleware doesn't seem like something usable in an existing application (https://github.com/MarkCiliaVincenti/TlsCertificateLoader/blob/377ff452f1bbf1d835a233534be4a4286f6d7248/TlsCertificateLoader/Extensions/TlsCertificateLoaderMiddlewareExtensions.cs#L107-L146), it kinda assumes it can add these middlewares to an existing pipeline without consequence.

https://github.com/MarkCiliaVincenti/TlsCertificateLoader/blob/e5f4fc31f1f29b3c5bf084aa04a0736cbc78b69d/Samples/CertbotSampleUsingMiddleware/Program.cs#L8-L10

@MarkCiliaVincenti
Copy link

MarkCiliaVincenti commented Nov 28, 2021 via email

@adityamandaleeka adityamandaleeka assigned HaoK and unassigned davidfowl May 12, 2022
@HaoK HaoK modified the milestones: 7.0-preview6, 7.0-preview7 Jun 20, 2022
@adityamandaleeka adityamandaleeka modified the milestones: 7.0-preview7, 7.0-rc1 Jul 6, 2022
@HaoK HaoK closed this as completed Aug 14, 2022
@HaoK HaoK added Done This issue has been fixed ✔️ Resolution: Fixed The bug or enhancement requested in this issue has been checked-in! labels Aug 14, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Sep 13, 2022
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affected-very-few This issue impacts very few customers api-approved API was approved in API review, it can be implemented area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions Done This issue has been fixed enhancement This issue represents an ask for new feature or an enhancement to an existing one ✔️ Resolution: Fixed The bug or enhancement requested in this issue has been checked-in!
Projects
None yet
Development

Successfully merging a pull request may close this issue.