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

SSLStream should support taking a pre-validated immutable full certificate chain #35844

Closed
davidfowl opened this issue May 5, 2020 · 9 comments · Fixed by #39818
Closed

SSLStream should support taking a pre-validated immutable full certificate chain #35844

davidfowl opened this issue May 5, 2020 · 9 comments · Fixed by #39818
Assignees
Milestone

Comments

@davidfowl
Copy link
Member

davidfowl commented May 5, 2020

In server scenarios on Linux it's important to support scenarios where the full certificate chain is provided outside of the certificate store (like a PEM file with the full chain). SSlStream should support this so that Kestrel can provide an API for providing the full certificate chain.

As an example, using Lets Encrypt with certbot generates both the SSL cert and a full chain cert. The latter is usually fed into Apache/nginx/haproxy (servers) which avoids the need for any need to retrieve the full chain on demand.

It also works well in container scenarios where the disk cache becomes ephemeral.

See dotnet/aspnetcore#21183 for the latest issue on an example where it was problematic.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Security untriaged New issue has not been triaged by the area owner labels May 5, 2020
@ghost
Copy link

ghost commented May 5, 2020

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq
Notify danmosemsft if you want to be subscribed.

@ghost
Copy link

ghost commented May 5, 2020

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

@davidfowl
Copy link
Member Author

This fits in with #31944

@iamcarbon
Copy link

We've done our own ACME implementation for a reverse proxy and were also given a lot of grief figuring out how to include the full chain. We finally figured it out but also ran into issues loading 5K+ certificates from memory and moved our front proxy to Caddy.

We'd love to see time invested in making the certificate selection async and ability to load 50K+ certificates with chains into memory (either at startup or lazily during certificate selection).

@mholt
Copy link

mholt commented May 5, 2020

@iamcarbon I'd like to know more about your deployment. Can I ask details? 😃 (So as not to derail this issue, could I email you? Or vice-versa?)

@davidfowl davidfowl changed the title SSLStream should support taking an X509Certicate2Collection with a full certificate chain SSLStream should support taking a pre-validated immutable full certificate chain May 17, 2020
@wfurt wfurt removed the untriaged New issue has not been triaged by the area owner label Jun 16, 2020
@wfurt wfurt added this to the 5.0.0 milestone Jun 16, 2020
@wfurt
Copy link
Member

wfurt commented Jul 20, 2020

Should SslStreamCertificateContext.Create throw if we cannot construct full chain or guarantie operation (on Windows)?
Or should we proceed as best-effort and possibly send incomplete chain and let client deal with it?
First option seems invasive but it would make issues more visible.
(In either case I will add tracing)

@vcsjones
Copy link
Member

throw if we cannot construct full chain or guarantie operation

My two cents: no. Clients can still potentially build a valid chain by performing their own AIA fetching, perhaps to a place where a server cannot fetch from. As I have expressed concern before, there is no “singular chain”. A client may end up building a different path, anyway.

Agree tracing is a useful though.

@bartonjs
Copy link
Member

As @vcsjones said, even if we're sending a sensible-looking chain there's no guarantee that the other side agrees it's sensible looking... and just because we send something partial doesn't mean it wasn't complete-enough for the remote, so throwing seems bad.

And, like everyone else, I think tracing seems valuable.

@karelz
Copy link
Member

karelz commented Aug 10, 2020

Fixed by #39818

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants