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

[HTTPS] Adds PEM support for Kestrel #23584

Merged
merged 9 commits into from
Jul 6, 2020
Merged

Conversation

javiercn
Copy link
Member

@javiercn javiercn commented Jul 1, 2020

  • Allows loading protected and unprotected PEM key files combined with a DER (crt) certificate

Open questions.

  • Is there a better way to detect the key type other than probing?
  • ECDsa support.

Addresses #4706

@ghost ghost added the area-servers label Jul 1, 2020
@javiercn javiercn requested a review from halter73 July 1, 2020 21:09
@javiercn javiercn force-pushed the javiercn/kestrel-pem-support branch from ef00f9f to fbe5489 Compare July 1, 2020 21:17
@javiercn javiercn marked this pull request as ready for review July 1, 2020 23:38
@JamesNK JamesNK requested a review from blowdart July 1, 2020 23:56
@blowdart
Copy link
Contributor

blowdart commented Jul 2, 2020

How long will ECC support take to add?

Copy link
Member

@davidfowl davidfowl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to discuss this a bit before merging @javiercn to make sure it aligns with the other features we're doing here.

@javiercn
Copy link
Member Author

javiercn commented Jul 2, 2020

How long will ECC support take to add?

Like 30 minutes? I forgot to add them in the first place

@blowdart
Copy link
Contributor

blowdart commented Jul 2, 2020

Hehe. OK if this gets updated for ECC then I'm good with it, once Jeremy's concerns are addressed

@javiercn
Copy link
Member Author

javiercn commented Jul 2, 2020

  • Removed the probing in favor of using the OIDs in the public key of the cert to figure out the private key type.
  • Added suppport for PEM cert + PEM Key
  • Check the certificate file to determine content type instead of invoking the X509Certificate2 constructor on random data.

@javiercn javiercn force-pushed the javiercn/kestrel-pem-support branch from 5bac8d3 to 2e68fd3 Compare July 2, 2020 20:35
@javiercn
Copy link
Member Author

javiercn commented Jul 2, 2020

🆙📅

The only thing left is to determine is if we need to handle more OIDs to support the current keys.

@javiercn
Copy link
Member Author

javiercn commented Jul 2, 2020

I'd like to discuss this a bit before merging @javiercn to make sure it aligns with the other features we're doing here.

We've discussed this offline, I've capture the feedback here. I think we are still missing some update on the runtime before we can load certificate collection/chains, but that should build on top of this PR.

Once we have an updated BCL with these APIs we can complete the work described in #23623
https://github.com/dotnet/runtime/blob/master/src/libraries/System.Security.Cryptography.X509Certificates/src/System/Security/Cryptography/X509Certificates/X509Certificate2Collection.cs#L339

I think this is ready to go % build passes

/cc: @davidfowl

@davidfowl davidfowl self-requested a review July 3, 2020 07:13
@davidfowl davidfowl dismissed their stale review July 3, 2020 07:14

Changes were made, discussions were had

@javiercn javiercn merged commit 9d3bf57 into master Jul 6, 2020
@javiercn javiercn deleted the javiercn/kestrel-pem-support branch July 6, 2020 13:48
@davidfowl
Copy link
Member

@javiercn a couple follow up items:

  • Certificate disposal (I see there’s an issue)
  • Documentation
  • Do we need a public API somewhere that can loads the pem cert and any key in the BCL?

@javiercn
Copy link
Member Author

javiercn commented Jul 6, 2020

  • Do we need a public API somewhere that can loads the pem cert and any key in the BCL?

There's LoadFromPemFile on che BCL, which is waht you would use.

@davidfowl
Copy link
Member

Why don’t we use it?

@javiercn
Copy link
Member Author

javiercn commented Jul 6, 2020

Why don’t we use it?

Cause the API is PEM + PEM and I wanted to also support loading the cert in DER format, so we load the CERT (PEM or DER) and then we load the key separately and marry it to the cert.

  • Documentation

dotnet/AspNetCore.Docs#19113

  • Certificate disposal (I see there’s an issue)

I would say if it is safe to double dispose certificates we just dispose all certificates on shutdown, otherwise we just dispose the ones loaded from config.

@davidfowl
Copy link
Member

Cause the API is PEM + PEM and I wanted to also support loading the cert in DER format, so we load the CERT (PEM or DER) and then we load the key separately and marry it to the cert.

Right, should we have an API for this? Or is it too niche?

@javiercn
Copy link
Member Author

javiercn commented Jul 6, 2020

Right, should we have an API for this? Or is it too niche?

I'm not sure how niche it is, PEM+PEM is likely way more popular, but I want to support if for convenience, so you don't have to use a tool to change the cert format. Some of the dev-servers that I used support this and it is convenient not to require a tool to change the format.

For example, you can use our dev cert with the angular dev server proxy by passing the cert in DER format and the key in PEM format, which is what we plan to do.

@bartonjs
Copy link
Member

bartonjs commented Jul 6, 2020

I would say if it is safe to double dispose certificates we just dispose all certificates on shutdown

Yep, certificate disposal is well-behaved.

logger.FailedToLoadCertificateKey(certificateKeyPath);
}

throw new InvalidOperationException(CoreStrings.InvalidPemKey);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could throw if the cert is missing, not just because the key was missing or invalid right? This exception message implies the key must be the problem?

And why do we use the exact same exception and log message any time the key is missing or invalid? It would be a lot better to log exactly why the key is invalid and to be very clear when the key is actually missing vs being invalid in both the exception and log messages.

@JamesNK JamesNK mentioned this pull request Sep 23, 2020
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants